Skip to content

Conversation

bbasata
Copy link
Collaborator

@bbasata bbasata commented May 2, 2025

Background: #269

This change is intended to unlock an upgrade path from v1.5.1 to latest v1.13.1.

When RefreshAfterApply is set to non-empty via a build flag, a Config-mode test step will invoke a refresh before successful completion. This is intended as a compatibility measure for test cases that have different -- but semantically-equal -- state representations in their test steps. When comparing two states, the testing framework is not aware of semantic equality or set equality, as that would rely on provider logic.

A Config-mode test step that is followed by a Refresh-mode test step will not incur an extra refresh. This is a backward-compatible way for a provider to eventually remove the compatibility flag – existing tests can be updated to explicitly refresh, incrementally.

cc: @BBBmau: I'd definitely like to get feedback from the provider team before shipping this. And it would be helpful to try this on a feature branch & see if it introduces any test failures.

@bbasata bbasata requested a review from a team as a code owner May 2, 2025 15:19
Copy link
Member

@austinvalle austinvalle left a comment

Choose a reason for hiding this comment

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

Yeah, like we chatted about offline, if this can be a stop gap before #327 is actually resolved, I'm on board 👍🏻

Other related issues and the original change:

@bbasata bbasata requested a review from a team as a code owner May 22, 2025 23:06

package resource

// Deprecated. This is an undocumented compatibility flag. When
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Deprecated

This is about the flexibility to remove this flag some day. There is no timetable or specific urgency for that. If we get feedback that this flag is super effective & we do not come up with a better alternative in the future, then I have no desire to break it 😃

Copy link
Member

@austinvalle austinvalle left a comment

Choose a reason for hiding this comment

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

Generally good with everything in this PR, since you prompted I would probably lean towards using an env variable, so if you're still keen to make that change I'll wait to approve

Copy link

@BBBmau BBBmau left a comment

Choose a reason for hiding this comment

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

I've done some TC tests and it looks to be good on the google side for addressing the verison pin, any chance we can get a release cut sometime soon? We can finish up MM support for ResourceIdentity once it's done.

@bbasata
Copy link
Collaborator Author

bbasata commented Jul 17, 2025

I've done some TC tests and it looks to be good on the google side for addressing the verison pin, any chance we can get a release cut sometime soon? We can finish up MM support for ResourceIdentity once it's done.

@BBBmau: I'm sorry for my delay in replying. I've now updated this branch to use a new environment variable: TF_ACC_REFRESH_AFTER_APPLY.

Would you update & give this one more run with your provider's suite for confidence before we release?

@BBBmau
Copy link

BBBmau commented Jul 25, 2025

@bbasata did some 2 nightly runs to confirm that things are good with the environment variable! We can go ahead with the release when you're ready.

austinvalle
austinvalle previously approved these changes Aug 13, 2025
Copy link
Member

@austinvalle austinvalle left a comment

Choose a reason for hiding this comment

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

Changes look good, one small nit.

We probably also want a changelog at some point before the release, if you'd prefer to do it in another PR that's cool too 👍🏻

// If the next step is a refresh, then we have no need to refresh here
if !c.Steps[stepIndex+1].RefreshState {
// Echo a searchable message to easily determine when this is no longer being used
fmt.Println(EnvTfAccRefreshAfterApply+":", "running apply -refresh-only -refresh=true")
Copy link
Member

Choose a reason for hiding this comment

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

nit: We should probably use the logging package for this with whatever log level you think is appropriate

@bbasata
Copy link
Collaborator Author

bbasata commented Aug 14, 2025

Changes look good, one small nit.

We probably also want a changelog at some point before the release, if you'd prefer to do it in another PR that's cool too 👍🏻

Done

nit: We should probably use the logging package for this with whatever log level you think is appropriate

and done!

@bbasata bbasata requested a review from austinvalle August 14, 2025 19:28
austinvalle
austinvalle previously approved these changes Aug 14, 2025
Copy link
Member

@austinvalle austinvalle left a comment

Choose a reason for hiding this comment

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

LGTM, the tests are failing but it doesn't look related 👍🏻

@bbasata bbasata merged commit 0fd5f69 into main Aug 14, 2025
42 checks passed
@bbasata bbasata deleted the gcp-debug branch August 14, 2025 20:26
bbasata added a commit that referenced this pull request Aug 14, 2025
)

* experimental: refresh after config mode test step

* fixup! experimental: refresh after config mode test step

* Skip an unnecessary refresh

* Rename compatibility flag and extract to compatibility.go

* Add header

* Use an environment variable

* logging > fmt

* Add changelog entry
bbasata added a commit that referenced this pull request Aug 15, 2025
) (#538)

* experimental: refresh after config mode test step

* fixup! experimental: refresh after config mode test step

* Skip an unnecessary refresh

* Rename compatibility flag and extract to compatibility.go

* Add header

* Use an environment variable

* logging > fmt

* Add changelog entry
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 13, 2025
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.

3 participants