Skip to content

fix(cloudformation): Hide deployment button when change set is not CR… #8338

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
Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

fix(cloudformation): Hide deployment button when change set is not CR… #8338

wants to merge 3 commits into from
+285 −16

Conversation

@chrisqm-dev
Copy link

@chrisqm-dev chrisqm-dev commented Nov 20, 2025

…EATE_COMPLETE and add delete button for CREATE_FAILED change sets

Problem

  • The "Deploy Changes" button is present when viewing a change set even if the change set does not have CREATE_COMPLETE status
  • Additionally, the "Delete Changeset" button is not present when no changes are detected in the change set

Solution

  • Added logic to check if the change set has CREATE_COMPLETE status before enabling the deploy changes button
  • Added the "Delete Changeset" button for the view when no changes are detected
    • Additionally, upon pressing the "Delete Changeset" button on the "no changes are detected" view, the button disappears to prevent additional delete attempts
  • Treat all work as PUBLIC. Private feature/x branches will not be squash-merged at release time.
  • Your code changes must meet the guidelines in CONTRIBUTING.md.
  • License: I confirm that my contribution is made under the terms of the Apache 2.0 license.

@chrisqm-dev chrisqm-dev requested a review from a team as a code owner November 20, 2025 22:15
@github-actions
Copy link

github-actions bot commented Nov 20, 2025

  • This pull request implements a feat or fix, so it must include a changelog entry (unless the fix is for an unreleased feature). Review the changelog guidelines.
    • Note: beta or "experiment" features that have active users should announce fixes in the changelog.
    • If this is not a feature or fix, use an appropriate type from the title guidelines. For example, telemetry-only changes should use the telemetry type.

tylerymarkowitz
tylerymarkowitz reviewed Nov 20, 2025
View reviewed changes
packages/core/src/awsService/cloudformation/commands/cfnCommands.ts Outdated
await environmentManager.refreshSelectedEnvironment()
} catch (error) {
getLogger().warn(`Failed to refresh selected environment: ${extractErrorMessage(error)}`)
getLogger().warn(`Failed to refresh seelcted environment: ${extractErrorMessage(error)}`)
Copy link

@tylerymarkowitz tylerymarkowitz Nov 20, 2025

Choose a reason for hiding this comment

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

typo?

Copy link
Author

@chrisqm-dev chrisqm-dev Nov 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

this change wasn't part of my PR, was a result of pulling old code, let me fix

Copy link
Contributor

@Zee2413 Zee2413 Nov 20, 2025

Choose a reason for hiding this comment

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

there are two typos from https://github.com/aws-cloudformation/cloudformation-vscode/pull/166/files

Copy link
Contributor

@Zee2413 Zee2413 Nov 20, 2025

Choose a reason for hiding this comment

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

would be good to address both

Copy link
Author

@chrisqm-dev chrisqm-dev Nov 21, 2025

Choose a reason for hiding this comment

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

The first typo was addressed when merging to this package, fixed the second typo in this PR

@chrisqm-dev chrisqm-dev marked this pull request as draft November 20, 2025 22:36
Zee2413
Zee2413 reviewed Nov 21, 2025
View reviewed changes
Copy link
Contributor

@Zee2413 Zee2413 left a comment

Choose a reason for hiding this comment

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

This needs a changelog: https://github.com/aws/aws-toolkit-vscode/blob/master/CONTRIBUTING.md#changelog

@chrisqm-dev chrisqm-dev force-pushed the fix/change-set-deploy-button branch from c612781 to 905c735 Compare November 21, 2025 12:54
@chrisqm-dev
Copy link
Author

chrisqm-dev commented Nov 21, 2025

This needs a changelog: https://github.com/aws/aws-toolkit-vscode/blob/master/CONTRIBUTING.md#changelog

Added changelog

@chrisqm-dev chrisqm-dev force-pushed the fix/change-set-deploy-button branch from 40e5561 to a2873d1 Compare November 21, 2025 13:00
@chrisqm-dev chrisqm-dev marked this pull request as ready for review November 21, 2025 18:48
Zee2413
Zee2413 reviewed Nov 21, 2025
View reviewed changes
packages/toolkit/.changes/next-release/Bug Fix-91b7177a-6dff-4062-bde6-21e3ba6efdf4.json Outdated
@@ -0,0 +1,4 @@
{
"type": "Bug Fix",
"description": "hide deployment button when change set is not deployable, add delete button when change set has no changes"
Copy link
Contributor

@Zee2413 Zee2413 Nov 21, 2025

Choose a reason for hiding this comment

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

I received recommendation to add prefix CloudFormation: to changelog to make it easier for release team/reviewers.

@chrisqm-dev chrisqm-dev force-pushed the fix/change-set-deploy-button branch 2 times, most recently from 6609e5f to f312c3b Compare November 21, 2025 21:40
Zee2413
Zee2413 reviewed Nov 21, 2025
View reviewed changes
packages/core/src/awsService/cloudformation/ui/diffWebviewProvider.ts
${
this.changeSetName &&
(this.changeSetStatus === ChangeSetStatus.CREATE_COMPLETE ||
this.changeSetStatus === ChangeSetStatus.FAILED)
Copy link
Contributor

@Zee2413 Zee2413 Nov 21, 2025

Choose a reason for hiding this comment

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

why are we including the button on failed?

Zee2413
Zee2413 reviewed Nov 21, 2025
View reviewed changes
packages/core/src/awsService/cloudformation/ui/diffWebviewProvider.ts
this.changeSetName &&
this.enableDeployments &&
(this.changeSetStatus === ChangeSetStatus.CREATE_COMPLETE ||
this.changeSetStatus === ChangeSetStatus.FAILED)
Copy link
Contributor

@Zee2413 Zee2413 Nov 21, 2025

Choose a reason for hiding this comment

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

why are we including failed then filter it out conditionally below?

@chrisqm-dev chrisqm-dev marked this pull request as draft November 21, 2025 21:52
@chrisqm-dev chrisqm-dev force-pushed the fix/change-set-deploy-button branch from f312c3b to 33bbce9 Compare November 25, 2025 19:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@Zee2413 Zee2413 Zee2413 left review comments

@tylerymarkowitz tylerymarkowitz tylerymarkowitz left review comments

Reviewers whose approvals may not affect merge requirements

At least 2 approving reviews are required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@chrisqm-dev @Zee2413 @tylerymarkowitz