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.

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

support pulumi cancel command #768

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

Conversation

hd-deman
Copy link

@hd-deman hd-deman commented Nov 1, 2022

No description provided.

@RobbieMcKinstry
Copy link
Contributor

Hi @hd-deman, thanks for opening this PR!

Could you remove the changes to dist/* from this PR? We auto-regenerate the build for each commit that hits the default branch.

Also, would you be willing to add a test to validate the change?

@hd-deman
Copy link
Author

hd-deman commented Nov 2, 2022

  1. removed dist from the commit
  2. no tests. but this fork was published
    and the up/cancel action was tested in a private CI project.

@RobbieMcKinstry
Copy link
Contributor

Thanks for removing the dist files, @hd-deman. I think we'll want to test this feature before we merge. I understand if you don't have the availability to add the test(s). Perhaps another community member would be willing to pick this up, or we, the maintainers, can do it when there's time (but I suspect this is a low-priority feature).

@hd-deman
Copy link
Author

hd-deman commented Nov 3, 2022

just a note. if you cancel pipeline it don't remove lock and break next deployments. So manual cancel action can solve this issue.

@hd-deman
Copy link
Author

hd-deman commented Nov 3, 2022

I mean, it doesn't look like a low priority

@dotansimha
Copy link

Any chance of getting this one merged soon? we wanted to use pulumi cancel in case of a CI cancellation

@RobbieMcKinstry
Copy link
Contributor

Hi @dotansimha this PR needs a few minor tweaks before we can get it in.

  • It requires a test case to verify correctness.
  • We need to add a changelog entry.
  • It needs to have the source reformatted; I'm seeing some misaligned text during code review.

@hd-deman Are you willing to add a test case? I'm looking for something that provably executes the cancel code. E.g. mock out stack.cancel(), call main configured with the cancel operation, and then verify that stack.cancel() has been called.

@RobbieMcKinstry
Copy link
Contributor

Also, I wanted to say I'm sorry if my comment about priority came off as rude. This is a valuable contribution that improves the quality of the Pulumi GitHub Action.

I want to get as many community contributions merged as possible. As an OSS maintainer, that's my job! Not every PR comes it 100% ready to merge; sometimes, they're 75% ready, 50% ready, or 25% ready. If the contributor can't take it all the way, the maintainers have to make up the difference, and there's only so much time in the day. Sometimes I have to choose between working on a 25% done, high-impact PR, and a 95% done, low-impact PR. At Pulumi, we measure "impact" using a couple of different factors, but the most relevant one is OSS user demand, which we measure by upvotes on the issue. When I said this issue was low-priority, I was referencing this imperfect measure of "impact". Since the PR doesn't have an associated open issue, it's at a disadvantage, since that means there aren't any upvotes (or others asking for the feature). It's still a valuable contribution nonetheless. Again, sorry if my comment was rude.

@@ -67,7 +67,8 @@ const main = async () => {
stack.refresh({ onOutput, ...config.options }).then((r) => r.stdout),
destroy: () =>
stack.destroy({ onOutput, ...config.options }).then((r) => r.stdout),
preview: async () => {
cancel: async () => { await stack.cancel(); return "" },
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this line needs to be aligned with the others.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/cicd kind/enhancement Improvements or new features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants