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

stacks: implement PlanTimestamp method on ExpressionScope #35367

Merged
merged 6 commits into from
Jun 26, 2024
Merged

Conversation

DanielMSchmidt
Copy link
Collaborator

@DanielMSchmidt DanielMSchmidt commented Jun 20, 2024

This PR adds plantimestamp to the stacks language. It persists the timestamp in the stack plan and it uses / forces the same timestamp across all components.

Fixes #

Target Release

1.10.x

Draft CHANGELOG entry

ENHANCEMENTS

  • Support plantimestamp in the stacks language

Copy link
Contributor

@apparentlymart apparentlymart left a comment

Choose a reason for hiding this comment

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

I see that this is a draft, so I haven't looked over it in full detail yet, but I just wanted to point out something I noticed on my first read that, if you change it in the way I proposed, would affect lots of different parts of this diff and so seemed worth raising now in case it would get harder to change after the remaining work.

Other than this I just want to make the broad note that for plantimestamp in the modules language we remember the timestamp we generated during the plan phase so that we can return exactly the same timestamp in the apply phase. I think we either need to do similarly here (saving the plan timestamp as part of the stack plan) or we'd need to mark the result of the function as ephemeral so that Terraform doesn't expect it to remain constant between the plan and apply phases. (But then it wouldn't really be the "plan timestamp" specifically anymore.)

internal/stacks/stackruntime/internal/stackeval/main.go Outdated Show resolved Hide resolved
@DanielMSchmidt DanielMSchmidt force-pushed the TF-10864 branch 2 times, most recently from bbd2664 to baa9f4e Compare June 21, 2024 14:32
@DanielMSchmidt DanielMSchmidt marked this pull request as ready for review June 24, 2024 15:19
This allows us to set the plantimestamp methods value in
the stacks language
Copy link
Member

@liamcervante liamcervante left a comment

Choose a reason for hiding this comment

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

I think one of the tests is not setting the planTimestamp

internal/stacks/stackruntime/internal/stackeval/main.go Outdated Show resolved Hide resolved
@DanielMSchmidt DanielMSchmidt merged commit 7520f46 into main Jun 26, 2024
10 checks passed
Copy link

Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch.

@DanielMSchmidt DanielMSchmidt deleted the TF-10864 branch June 26, 2024 10:03
Copy link

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 Jul 27, 2024
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.

None yet

3 participants