fix(toolkit-lib): every change-set deployment announces "waiting in review for manual execution (--no-execute)" although the change set is executed

[go-to-k]

Fixes #1815

Problem

Since #1273, the deploy action always creates the change set upfront (to get an accurate diff for the approval prompt) by calling Deployments.prepareStack(), which internally forces execute: false, and then executes the prepared change set in a second phase. deploy-stack.ts prints

Changeset ... created and waiting in review for manual execution (--no-execute)

whenever execute is false, so every change-set deployment with actual changes now prints this message and then executes the change set anyway. The message is misleading: --no-execute was never requested, and the change set does not wait for anything. This also surfaces in flows that deploy internally, e.g. the finalizing deployment after cdk import.

Fix

deploy-stack.ts cannot know whether execute: false is user-requested or the internal first phase of an executing deployment, so the caller now tells it, via a single intent-revealing option: willExecuteChangeSet (default false: the change set is the user's final artifact). Both behaviors that depend on this intent are derived from the one flag:

• deploy-stack.ts announces a created-but-not-executed change set as waiting for manual execution only when willExecuteChangeSet is not set (this is the fix);
• Deployments.prepareStack() cleans up an empty change set only when willExecuteChangeSet is set (this replaces the former cleanupOnNoOp option, which encoded the same fact under a behavior-specific name).

The deploy flows (the toolkit deploy action and the CLI's deploy flow, which share prepareStack) pass willExecuteChangeSet: isExecutingChangeSetDeployment(deploymentMethod); prepareStack passes the flag through to deployStack unchanged. The default preserves today's behavior for every other caller, including cdk import --no-execute, which keeps its announcement without any change to the importer.

The option lives on the internal DeployStackOptions interfaces (api/deployments is not part of the toolkit-lib public API), so there is no public API change.

Alternative considered

An alternative implementation was initially committed (e2c6083): removing the announcement from deploy-stack.ts entirely and emitting it from the callers that know the user's intent (prepareStack and the resource importer). Its appeal is that it structurally removes deploy-stack's ability to assert a user intent it cannot know.

It was replaced with the current approach because:

• the announcement stays at a single emission site, at the moment the change set is created;
• the importer is left untouched (cdk import --no-execute keeps its announcement via the default, instead of re-implementing it);
• the default preserves today's behavior for every caller of deployStack with execute: false.

Both directions are kept in the branch history.

Testing

• New unit tests in test/api/deployments/deploy-stack.test.ts: with execute: false, the change set is announced as awaiting manual execution by default, and not announced with willExecuteChangeSet: true.
• New unit tests in test/api/deployments/cloudformation-deployments.test.ts: prepareStack passes willExecuteChangeSet through to deployStack unchanged (set for the internal prepare of an executing deployment, unset for a user-requested --no-execute prepare).
• Verified against a real AWS environment:
  • CLI 2.1135.0 (released): a plain cdk deploy with changes prints the misleading message and then executes the change set.
  • This branch: the same deploys (both a stack creation and an update with changes) print no such message and execute normally; cdk deploy --method=prepare-change-set still prints the message and leaves the change set unexecuted (stack UPDATE_COMPLETE and unchanged, change set CREATE_COMPLETE/AVAILABLE).

---

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.32%. Comparing base (785506e) to head (d4e3f5f).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1818   +/-   ##
=======================================
  Coverage   90.32%   90.32%           
=======================================
  Files          80       80           
  Lines       12124    12124           
  Branches     1716     1716           
=======================================
  Hits        10951    10951           
  Misses       1139     1139           
  Partials       34       34           

Flag	Coverage Δ
suite.unit	90.32% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[9pace]

Thanks for the contribution! You have correctly identified an issue here.

on announceNoExecuteChangeSet:

• PrepareStackOptions extends Omit<DeployStackOptions, 'deploymentMethod'>, so it inherits announceNoExecuteChangeSet, but prepareStack() overwrites it after the spread with !options.cleanupOnNoOp. A caller-supplied value is ignored.

• cleanupOnNoOp and announceNoExecuteChangeSet are two booleans derived from one underlying fact: whether the change set is the user's final artifact (--no-execute) or the first phase of a deployment that will execute it. The proposed change (Syncing them with an inversion at one call site) invites drift, and the announce flag is named for the message it controls rather than the intent behind it.

Suggestion: Rename cleanupOnNoOp to something intent-revealing (e.g. willExecuteChangeSet) and derive both behaviors (empty-change-set cleanup and announcement suppression) from it.

[9pace]

If caller passed announceNoExecuteChangeSet here it would be ignored. Effectively the options.announceNoExecuteChangeSet is unused.

[go-to-k]

Thanks for the review! Applied in d4e3f5f: renamed cleanupOnNoOp to willExecuteChangeSet and derived both behaviors from that single flag.