actions icon indicating copy to clipboard operation
actions copied to clipboard

support pulumi cancel command

Open hd-deman opened this issue 3 years ago • 8 comments

hd-deman avatar Nov 01 '22 21:11 hd-deman

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?

RobbieMcKinstry avatar Nov 02 '22 01:11 RobbieMcKinstry

  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.

hd-deman avatar Nov 02 '22 10:11 hd-deman

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).

RobbieMcKinstry avatar Nov 03 '22 17:11 RobbieMcKinstry

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 avatar Nov 03 '22 17:11 hd-deman

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

hd-deman avatar Nov 03 '22 17:11 hd-deman

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

dotansimha avatar Dec 18 '22 16:12 dotansimha

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 avatar Dec 19 '22 15:12 RobbieMcKinstry

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.

RobbieMcKinstry avatar Dec 19 '22 16:12 RobbieMcKinstry