introduce service hooks #12166
introduce service hooks #12166
Conversation
ndeloof
requested review from
a team,
glours and
jhrotko
and removed request for
a team
ndeloof
force-pushed
the
service_hooks
branch
4 times, most recently
from
b870947
to
0aacddf
Compare
go.mod
Outdated
| @@ -193,4 +193,4 @@ require ( | |||
| tags.cncf.io/container-device-interface v0.8.0 // indirect | |||
| ) | |||
|
|
|||
| replace github.com/compose-spec/compose-go/v2 => github.com/compose-spec/compose-go/v2 v2.2.1-0.20240926141145-d3fd7d94aa31 | |||
| replace github.com/compose-spec/compose-go/v2 => github.com/ndeloof/compose-go/v2 v2.0.1-0.20240924090029-29a100a62f5f | |||
There was a problem hiding this comment.
leaving this comment here not to forget to delete this after compose-spec is bumped
| for _, hook := range service.PreStop { | ||
| err := s.runHook(ctx, container, *service, hook, nil) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| } |
There was a problem hiding this comment.
We are always iterating over all hooks for PreStop, PostStart, etc.
I think it would make sense to add a function that encapsulates this in hook.go.
func (s composeService) ExecuteHooks(ctx context.Context, container moby.Container, service types.ServiceConfig, listener api.ContainerEventListener, hooks []types.ServiceHook) error {
for _, hook := range hooks {
err := s.runHook(ctx, container, service, hook, listener)
if err != nil {
return err
}
}
return nil
}
| @@ -85,7 +85,8 @@ func (s *composeService) down(ctx context.Context, projectName string, options a | |||
|
|
|||
| err = InReverseDependencyOrder(ctx, project, func(c context.Context, service string) error { | |||
| serviceContainers := containers.filter(isService(service)) | |||
| err := s.removeContainers(ctx, serviceContainers, options.Timeout, options.Volumes) | |||
| serv := project.Services[service] | |||
There was a problem hiding this comment.
We could add a validation that the service exists here. Since this is the place where we get its value becoming easier later on for debugging.
serv, ok := project.Services[service]
if !ok {
return fmt.Error() ....
}
There was a problem hiding this comment.
as we run inside InReverseDependencyOrder service is guaranteed to exist (actually, InReverseDependencyOrder could pass a *ServiceConfig, but this could have impact on immutability)
service is passed as a pointer down to
Line 304 in 0aacddf
nil
| case <-tick.C: | ||
| inspect, err := s.apiClient().ContainerExecInspect(ctx, exec.ID) | ||
| if err != nil { | ||
| return nil |
There was a problem hiding this comment.
Might have missed something, but shouldn't we return the error in this case? I see the same is done for ContainerExecStart. But in line 84 we do not ignore these errors
| @@ -309,32 +320,32 @@ func (s *composeService) stopContainer(ctx context.Context, w progress.Writer, c | |||
| return nil | |||
| } | |||
|
|
|||
| func (s *composeService) stopContainers(ctx context.Context, w progress.Writer, containers []moby.Container, timeout *time.Duration) error { | |||
| func (s *composeService) stopContainers(ctx context.Context, w progress.Writer, serv *types.ServiceConfig, containers []moby.Container, timeout *time.Duration) error { | |||
There was a problem hiding this comment.
nit: rename to serv -> service for consistency
| wOut := utils.GetWriter(func(line string) { | ||
| listener(api.ContainerEvent{ | ||
| Type: api.HookEventLog, | ||
| Container: getContainerNameWithoutProject(container) + " ->", |
There was a problem hiding this comment.
I would propose to change the -> to the name of the hook + index? we could pass it as a variable. If all hooks have the same "signature" -> might be complicated to understand from which hook the log comes from
There was a problem hiding this comment.
hooks have no name so far (to be debated)
I wanted here to distinguish hook output from main logs without making the prefix a looooooong line, as this will remain for the whole compose execution after hook completed
There was a problem hiding this comment.
how about we choose an emoji for each hook? 😆
There was a problem hiding this comment.
My first implementation was using 🪝 but this won't run well on a Windows terminal
|
Really cool stuff @ndeloof 😊 |
Signed-off-by: Nicolas De Loof <[email protected]>
What I did
Added support for
post_startandpre_stoplifecycle hooks to servicesdemo: https://asciinema.org/a/Oh1ZFu2IERd3Fo4nK6lcSAhwS
Related issue
(not mandatory) A picture of a cute animal, if possible in relation to what you did
