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

Add deletion of stack after destroy (downsert/remove) #759

Merged
merged 6 commits into from
Nov 17, 2022
Merged

Add deletion of stack after destroy (downsert/remove) #759

merged 6 commits into from
Nov 17, 2022

Conversation

Moon1706
Copy link
Contributor

Hi folks! We've already discussed this feature previously, but I just didn't have time to continue. So, here I am. As I can see, action wasn't super reorganized and appending delete stack flag is relevant. Thus, what do you think about the new field?

@RobbieMcKinstry
Copy link
Contributor

Hi again @Moon1706 . Good to hear from you!
Thanks for this PR. It looks more manageable than the last one which had both downsert and version 👍🏻 (since it was from your upstream fork 😄).

I'm in favor of adding a downsert option. Personally, I don't like the name downsert since it's not clear if it refers to the stack or the state. Others have suggested --rm for the CLI.

However, given that we already have upsert, I think we should go ahead with downsert. We can always deprecate the name later if the CLI supports a different term. I asked around internally to see what other people think.

Copy link
Contributor

@RobbieMcKinstry RobbieMcKinstry left a comment

Choose a reason for hiding this comment

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

Before merging the PR, I'd love it if we could add two tests:

  1. Demonstrate that stack.removeWorkspace is called when downsert: true.
  2. Demonstrate that stack.removeWorkspace isn't called when downsert is not provided.

Lastly, it would be great if we could log a warning if the user has downsert: true but command is set to anything other than destroy.

.github/workflows/test.yml Outdated Show resolved Hide resolved
.github/workflows/workflow.yml Outdated Show resolved Hide resolved
.github/workflows/workflow.yml Outdated Show resolved Hide resolved
.github/workflows/workflow.yml Outdated Show resolved Hide resolved
.github/workflows/workflow.yml Outdated Show resolved Hide resolved
.github/workflows/workflow.yml Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
action.yml Outdated Show resolved Hide resolved
@RobbieMcKinstry
Copy link
Contributor

Ah, TIL we recently added support for --remove in the CLI, serving the same function as downsert. Can we stick with that naming?
pulumi/pulumi#10484

@RobbieMcKinstry RobbieMcKinstry added kind/enhancement Improvements or new features impact/usability Something that impacts users' ability to use the product easily and intuitively area/cicd labels Oct 18, 2022
@RobbieMcKinstry RobbieMcKinstry self-assigned this Oct 18, 2022
@RobbieMcKinstry RobbieMcKinstry linked an issue Oct 18, 2022 that may be closed by this pull request
@RobbieMcKinstry RobbieMcKinstry changed the title Add deletion of stack (downsert) Add deletion of stack after destroy (downsert/remove) Oct 18, 2022
@Moon1706
Copy link
Contributor Author

Ah, TIL we recently added support for --remove in the CLI, serving the same function as downsert. Can we stick with that naming? pulumi/pulumi#10484

Yeah, I've checked the discussion. Mmm, I'm not sure. This option doesn't exist in @pulumi/pulumi/automation.

@RobbieMcKinstry
Copy link
Contributor

RobbieMcKinstry commented Oct 18, 2022

@Moon1706 You can still keep the code / logic the same, but could you rename the input? I.e. change downsert in the config to remove.

@Moon1706
Copy link
Contributor Author

@Moon1706 You can still keep the code / logic the same, but could you rename the input? I.e. change downsert in the config to remove.

Sure) I'm just in UTC+4 time zone, and gonna apply this suggestion tomorrow.

@Moon1706
Copy link
Contributor Author

Moon1706 commented Oct 18, 2022

@RobbieMcKinstry
I'm wondering maybe we can open an issue in the official Pulumi repo about adding remove argument to the stack file. What do you think about it?

@RobbieMcKinstry
Copy link
Contributor

Hey @Moon1706 sorry for the slow reply, once again.

I think we should move forward with this PR with or without the related PR to automation API. We can always switch to the AutoAPI implementation later once it lands, but there's no need to wait for that.

The only blocker is to rename downsert to remove to be consistent with the CLI.

Copy link
Contributor

@RobbieMcKinstry RobbieMcKinstry left a comment

Choose a reason for hiding this comment

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

Here's a changeset that renames downsert to remove.

README.md Outdated Show resolved Hide resolved
action.yml Outdated Show resolved Hide resolved
src/__tests__/config.test.ts Outdated Show resolved Hide resolved
src/__tests__/config.test.ts Outdated Show resolved Hide resolved
src/__tests__/config.test.ts Outdated Show resolved Hide resolved
src/config.ts Outdated Show resolved Hide resolved
src/config.ts Outdated Show resolved Hide resolved
src/main.ts Outdated Show resolved Hide resolved
@RobbieMcKinstry
Copy link
Contributor

RobbieMcKinstry commented Nov 15, 2022

@Moon1706 If these changes look good, I'll commit them and merge this PR.

@Moon1706
Copy link
Contributor Author

@RobbieMcKinstry
I've added. Check please

@Moon1706
Copy link
Contributor Author

But yeah, changes LGTM. Will we wait for applying changes to the main Pulumi repo?

Copy link
Contributor

@RobbieMcKinstry RobbieMcKinstry left a comment

Choose a reason for hiding this comment

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

LGTM! I think status checks are failing because there's no changelog entry. That's the last step! (Sorry, I should have mentioned that before...)

@RobbieMcKinstry
Copy link
Contributor

RobbieMcKinstry commented Nov 15, 2022

@Moon1706

Will we wait for applying changes to the main Pulumi repo?

No, I think this is good to go! :) Could you add a changelog entry?

@Moon1706
Copy link
Contributor Author

@RobbieMcKinstry Aahah) My bad) I thought you were talking about the main changelogs of the Pulumi repository. Okay, give me 5 minutes, I'll add it.

@RobbieMcKinstry RobbieMcKinstry merged commit a8f22a7 into pulumi:master Nov 17, 2022
@RobbieMcKinstry
Copy link
Contributor

Thanks for another contribution Moon! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/cicd impact/usability Something that impacts users' ability to use the product easily and intuitively kind/enhancement Improvements or new features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add option to delete stack on pulumi destroy [pulumi stack rm]
3 participants