Add "Restart Piped" command handler to launcher #3745
Conversation
| } | ||
| } | ||
| } | ||
| go commandHandler(ctx, l.commandCh) |
There was a problem hiding this comment.
How about using goroutine with errorGroup instead of creating a simple goroutine?
| select { | ||
| case <-ticker.C: | ||
| // Don't return an error to continue piped execution. | ||
| l.enqueueNewCommands(ctx, input) |
There was a problem hiding this comment.
There was a problem hiding this comment.
I got your point and I feel it's better but it's up to you since there are already some implementations like this. 🙌
| ) | ||
| logger.Info("received a restart piped command to handle") | ||
|
|
||
| l.detectRestartCommand = true |
There was a problem hiding this comment.
Just feel like this one is not an effective way to implement this feature, we need one ticker (to fetch the commands) and one other ticker(util launcher self checkInterval) to restart the piped. Should we rewrite some part where we restart piped by launcher to reuse that?
ref: current rerun logic https://github.com/pipe-cd/pipecd/blob/master/pkg/app/launcher/cmd/launcher/launcher.go#L248-L262
There was a problem hiding this comment.
we can keep the use of this l.detectRestartCommand flag but instead of forking a new go routine to just change this flag value, we can make this in the current enqueueNewCommands, and in execute function, we have shouldRelaunch() function which check for the flag and restart the piped for us 🤔
…3534-restart-piped-launcher
|
duplicated with #3831 so close this |
Pull request was closed
3 participants
What this PR does / why we need it:
According to the #3534, it's useful that there is the feature to restart piped from the web console.
To implement this feature, I have added a command handler to handle RestartPiped Command to launcher as a continuation of #3725
Note that this PR does not implement any specific API or UI changes, so there are no user-facing changes.
Which issue(s) this PR fixes:
Fixes partially #3534
Does this PR introduce a user-facing change?: