Fix cloud integration's handling of TF_WORKSPACE environment variable#34012
Merged
nfagerlund merged 8 commits intomainfrom Oct 9, 2023
Merged
Fix cloud integration's handling of TF_WORKSPACE environment variable#34012nfagerlund merged 8 commits intomainfrom
nfagerlund merged 8 commits intomainfrom
Conversation
nfagerlund
changed the title
This will be the central location for everything involving combining environment variables with a `cloud` config block to obtain a final cloud config. It returns a plain Go value (so that nothing downstream of it ever needs to mess with Cty types), and doesn't mutate any fields on the backend, so it has a nice firm boundary of responsibilities. Also, it's quite a bit more pedantic and explicit about HOW the environment variables get consulted, in the hope of reducing future misunderstandings about our UI-level expectations.
This fixes issue #33976, introduced in #33489, which broke the intended behavior of specifying the active workspace via the TF_WORKSPACE variable when using a tag-based workspace mapping. Now that all the default and fallback value behaviors are cleanly isolated in a function, this whole flow can be a bit simpler. - Remove `setConfigurationFields`. Instead, `Configure` can just trade its Cty `obj` for a dumb struct and set a couple fields from it. The `TF_FORCE_LOCAL_BACKEND` handling can just join the relevant section of Configure directly. - Radically chop down PrepareConfig. It turns out we were violating the interface contract, which says PrepareConfig shouldn't trouble itself with the shell environment and fallback values... So, don't do that.
nfagerlund
force-pushed
the
nf/oct23-tf_workspace-problems
branch
from
a05e904 to
fec81be
Compare
These tests were rather chaotic, because the basic job of validating the finalized config was split across a bunch of different places. Now they're in (mostly) one spot, testing `resolveCloudConfig`. They might be somewhat excessive or redundant, and can be revised later. In a few cases, I revised test cases that had incorrect expectations, to match the fixed implementation. Also, note that the new implementation is less loosey-goosey around cty values, and expects something that definitely matches the schema (though any amount of it might be null). This commit also splits the TF_FORCE_LOCAL_BACKEND test case into a separate test function.
These got turned off in d7e07e6 by accident
That error happens after PrepareConfig, now.
Actually I don't think these _ever_ worked.
nfagerlund
force-pushed
the
nf/oct23-tf_workspace-problems
branch
from
5fbd1c8 to
438e476
Compare
nfagerlund
added
the
1.6-backport
brandonc
approved these changes
Oct 9, 2023
Contributor
brandonc
left a comment
brandonc
left a comment
There was a problem hiding this comment.
Works great and nice refactor.
| } | ||
|
|
||
| func WithEnvVars(t *testing.T) { | ||
| func TestCloud_configWithEnvVars(t *testing.T) { |
Contributor
|
Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch. |
ghost
mentioned this pull request
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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #33976
This bug was introduced in #33489 (adding
projectto the cloud config). Analyzing the flow of logic around config handling for the cloud backend took, a long time, which convinced me that a bug like this was honestly pretty inevitable — this code was too complex to safely edit.So, this PR refactors that whole zone to draw some sharper divisions of responsibility:
PrepareConfigdoes almost nothing, as per the interface contract (which we were previously ignoring).Configureno longer delegates to the intensely side-effectfulsetConfigurationFieldshelper method to set fields on self; instead it just trades its cty config object for a dumb Go struct, sets a couple fields, and moves on.Notes for reviewers:
Target Release
1.6.1, as this bug was a disruptive regression.
Draft CHANGELOG entry
BUG FIXES
TF_WORKSPACEenvironment variable once again works properly for selecting the active workspace when using thecloudblock with workspacetags.