Notify user if deployment will not be cancelled when terminating. #875
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jordanstephens
commented
Nov 20, 2024
| go func() { | ||
| <-ctx.Done() | ||
|
|
||
| if errors.Is(ctx.Err(), context.Canceled) || errors.Is(ctx.Err(), context.DeadlineExceeded) { |
There was a problem hiding this comment.
This is not enough to exclude the natural cancellation of the context from success :(
defang up
* Using Defang Playground provider from stored preference
* Packaging the project files for app at /Users/jordan/wk/samples/samples/nodejs-http/app
* Uploading the project files for app
* Monitor your services' status in the defang portal
- https://portal.defang.dev/service/app
* Tailing logs for deployment ID qqut567fztmx ; press Ctrl+C to detach:
* Showing only build logs and runtime errors. Press V to toggle verbose mode.
2024-11-19T15:52:44.499-08:00 cd Update started for stack jordanstephens-prod1
2024-11-19T15:52:48.671-08:00 cd ** Building image for "app"...
2024-11-19T15:52:51.878-08:00 cd ** Build succeeded for "app"
2024-11-19T15:52:52.356-08:00 cd ** Updated service "app" to revision 50
2024-11-19T15:52:56.752-08:00 cd Update succeeded in 12.284256189s ; provisioning...
2024-11-19T15:53:39.389-08:00 app Server running at http://127.0.0.1:3000/
errCompleted
! Deployment will not be cancelled.
* Service app is in state DEPLOYMENT_COMPLETED and will be available at:
- https://jordanstephens-app--3000.prod1a.defang.dev
* Done.
For help with warnings, check our FAQ at https://docs.defang.io/docs/faq
There was a problem hiding this comment.
So does that mean this PR is not ready to be merged? I wouldn't want the happy path to show "Deployment will not be cancelled".
There was a problem hiding this comment.
Right this PR is not ready to merge. It's still in draft.
lionello
reviewed
Nov 26, 2024
Comment on lines
-160
to
-163
| if errors.Is(context.Cause(tailCtx), context.Canceled) { | ||
| // Tail was canceled by the user before deployment completion/failure; show a warning and exit with an error | ||
| term.Warn("Deployment is not finished. Service(s) might not be running.") | ||
| return err |
| go func() { | ||
| <-ctx.Done() | ||
|
|
||
| if errors.Is(ctx.Err(), context.Canceled) || errors.Is(ctx.Err(), context.DeadlineExceeded) { |
There was a problem hiding this comment.
So does that mean this PR is not ready to be merged? I wouldn't want the happy path to show "Deployment will not be cancelled".
| <-ctx.Done() | ||
|
|
||
| if errors.Is(ctx.Err(), context.Canceled) || errors.Is(ctx.Err(), context.DeadlineExceeded) { | ||
| term.Warn("Deployment will not be cancelled.") |
There was a problem hiding this comment.
This message could be less curt.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
We have been printing a message
Detached. The process will keep runningif the deployment process was terminated after the tail began, but this left a small window of time (between the deployment request being sent and the tail being initiated) where termination of the process would fail to cancel the deployment and we would not notify the user. This change covers that gap by sharing a cancellable context for both sending the deployment request and the tail.Linked Issues
Checklist