Skip to content

stacks: follow component progress cycle for empty destroys#38049

Merged
DanielMSchmidt merged 2 commits intomainfrom
TF-31135
Jan 15, 2026
Merged

stacks: follow component progress cycle for empty destroys#38049
DanielMSchmidt merged 2 commits intomainfrom
TF-31135

Conversation

@DanielMSchmidt
Copy link
Copy Markdown
Contributor

@DanielMSchmidt DanielMSchmidt commented Jan 9, 2026

We expect a component instances to report its plan/apply starting and ending as well as reporting the progress / result. This should also be the case for no-ops like an empty component instance.

Fixes TF-31135

Target Release

1.15.x

Rollback Plan

  • If a change needs to be reverted, we will roll out an update to the code within 7 days.

Changes to Security Controls

Are there any changes to security controls (access controls, encryption, logging) in this pull request? If so, explain.

CHANGELOG entry

  • This change is user-facing and I added a changelog entry.
  • This change is not user-facing.

@DanielMSchmidt DanielMSchmidt added the no-changelog-needed Add this to your PR if the change does not require a changelog entry label Jan 9, 2026
@DanielMSchmidt DanielMSchmidt changed the title stacks: add tests around apply time progress messages stacks: follow component progress cycle for empty destroys Jan 12, 2026
@DanielMSchmidt DanielMSchmidt marked this pull request as ready for review January 12, 2026 13:24
@DanielMSchmidt DanielMSchmidt requested a review from a team as a code owner January 12, 2026 13:24
dsa0x
dsa0x previously approved these changes Jan 12, 2026
}

func TestStackChangeProgress(t *testing.T) {
func TestStackChangeProgressDuringPlan(t *testing.T) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is nitpicky and you can choose to ignore it, but since I'm fighting with similarly complicated-to-refactor-or-debug tests in actions right now, I'd suggest that you make a new test instead of adding a mode flag.

These tests get harder and harder to work with the more the test harness varies between test cases (or maybe the more the test cases vary within the harness? the more flags we have, is what I'm saying 😂); it's easier to refactor or add new features when tests cover different functionality, instead of all the functions in one test.

I'd peresonally much rather see two tests with names like TestStackChangeProgressDuringPlan, TestStackChangeProgressDuringPlan_destroy than one test that's covering all modes; it's a lot easier to read and understand what's being testing in each individual test that way, and it's easier to debug test failures during development when a single test isn't covering a number of different codepaths.

nfagerlund
nfagerlund previously approved these changes Jan 13, 2026
Copy link
Copy Markdown
Member

@nfagerlund nfagerlund left a comment

Choose a reason for hiding this comment

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

Awesome! I did an interactive test with a terraform built from this PR branch, and empty destroy applies (from a destroy convergence check) look great:

Image

Likewise with empty destroys due to a deployment being left in the config with destroy = true. This yields good behavior with no changes made to the agent or frontend.

(I don't know why the merge target is set to another PR branch rather than main, but I'm guessing it's on purpose to orchestrate a series of merges or something.)

@DanielMSchmidt DanielMSchmidt requested review from a team as code owners January 15, 2026 09:22
@DanielMSchmidt DanielMSchmidt changed the base branch from TF-30650 to main January 15, 2026 09:28
@DanielMSchmidt DanielMSchmidt dismissed stale reviews from nfagerlund and dsa0x January 15, 2026 09:28

The base branch was changed.

We expect a component instances to report its plan/apply starting and ending as
well as reporting the progress / result. This should also be the case for no-ops
like an empty component instance.
@DanielMSchmidt
Copy link
Copy Markdown
Contributor Author

Good point @nfagerlund, there was no good reason to stack the PRs (pun intended). I at first thought the issue was similar to the other PRs and thus I borrowed some test enhancements. In the end it was not dependent. I moved the related commits to the other PR and kept this one focussed.

@DanielMSchmidt DanielMSchmidt removed the no-changelog-needed Add this to your PR if the change does not require a changelog entry label Jan 15, 2026
@DanielMSchmidt DanielMSchmidt enabled auto-merge (rebase) January 15, 2026 13:20
@DanielMSchmidt DanielMSchmidt merged commit b926263 into main Jan 15, 2026
11 checks passed
@DanielMSchmidt DanielMSchmidt deleted the TF-31135 branch January 15, 2026 13:23
@github-actions
Copy link
Copy Markdown
Contributor

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 15, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants