Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(stop): allow to also cancel any ongoing deployment #766

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

feat(stop): allow to also cancel any ongoing deployment #766

wants to merge 1 commit into from

Conversation

davlgd
Copy link
Contributor

@davlgd davlgd commented Jun 24, 2024

This PR adds --cancel and -c to the stop command. It allows to ask to stop the app and to cancel any ongoing deployment with only one command.

The branch has been rebased on support-app-id from Pierre

@davlgd davlgd self-assigned this Jun 24, 2024
@davlgd davlgd requested a review from a team as a code owner June 24, 2024 20:40
Base automatically changed from support-app-id to master June 26, 2024 12:57
An error occurred while trying to automatically change base from support-app-id to master June 26, 2024 12:57
hsablonniere
hsablonniere reviewed Jun 26, 2024
View reviewed changes
Copy link
Member

@hsablonniere hsablonniere left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 I'm OK with the code but I want to discuss the UX...

  • a clever stop --cancel fails if there is no ongoing deployment
  • why no clever stop && clever cancel-deploy?
  • is --cancel clear enough? should it be clever stop --cancel-deploy?

@github-actions GitHub Actions
Copy link

github-actions bot commented Jun 26, 2024 •
edited
Loading

🔎 A preview has been automatically published:

  • 🐧 linux b1a9ed6467383f3cba3fc321d777617e491cae21d436b4076cf45fefe4e5fe4b
  • 🍏 macos 886c8fded801731fa62cb4db69bbb208541a88cedf0db0e9df9f2ef29a5d0b84
  • 🪟 win 3006f82ee7a3d92f6f70ce9faaead6d0c71b99b65fb68622a5acba78004e3b26

This preview will be deleted once this PR is closed.

@hsablonniere hsablonniere added this to the Next release milestone Jun 26, 2024
@davlgd
Copy link
Contributor Author

davlgd commented Jun 26, 2024

👍 I'm OK with the code but I want to discuss the UX...

  • a clever stop --cancel with fail if there is no ongoing deployment
  • why no clever stop && clever cancel-deploy?
  • is --cancel clear enough? should it be clever stop --cancel-deploy?

A clever stop && clever cancel-deploy is what I try to avoid here. And I'm often in the case where I want to stop every deployment of an app, it's why I did this at first.

For the flag name I'm open. Help is clear enough but the command is cancel-deploy. We can also use a more generic --force for example.

@davlgd davlgd force-pushed the davlgd-stop-with-cancel branch 2 times, most recently from 802586c to 2ab0d72 Compare June 27, 2024 16:53
@davlgd davlgd requested a review from hsablonniere June 27, 2024 16:54
@davlgd
Copy link
Contributor Author

davlgd commented Jun 27, 2024

As discussed in sync it's now:

  • --all flag
  • Don't cancel-deploy if there is no ongoing deployment, without throwing an error
  • Rebased on master

aurrelhebert
aurrelhebert approved these changes Jun 28, 2024
View reviewed changes
Copy link
Member

@aurrelhebert aurrelhebert left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! GJ looks clean to me

I think that the

clever stop --all --app {APP_ID}

will be really useful 💖

hsablonniere
hsablonniere requested changes Jul 1, 2024
View reviewed changes
Copy link
Member

@hsablonniere hsablonniere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think a command should import another command. I'll rework the commit myself.

hsablonniere
hsablonniere requested changes Jul 1, 2024
View reviewed changes
Copy link
Member

@hsablonniere hsablonniere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the code so a command does not rely on another but I'm not sure this update is necessary.

From my tests, a stop on an app cancels any ongoing deployments (new commits and upscaling). Let's do some more tests before merging this...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hsablonniere hsablonniere hsablonniere requested changes

@aurrelhebert aurrelhebert aurrelhebert approved these changes

Requested changes must be addressed to merge this pull request.

Assignees

@davlgd davlgd

Labels
None yet
Projects
None yet
Milestone
After rework/refacto
Development

Successfully merging this pull request may close these issues.
3 participants
@davlgd @hsablonniere @aurrelhebert