-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
Fix cloud integration's handling of TF_WORKSPACE environment variable #34012
Conversation
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.
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.
5fbd1c8
to
438e476
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works great and nice refactor.
@@ -138,7 +138,7 @@ func TestCloud_PrepareConfig(t *testing.T) { | |||
} | |||
} | |||
|
|||
func WithEnvVars(t *testing.T) { | |||
func TestCloud_configWithEnvVars(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❗
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rite?
Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch. |
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. |
Fixes #33976
This bug was introduced in #33489 (adding
project
to 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:
PrepareConfig
does almost nothing, as per the interface contract (which we were previously ignoring).Configure
no longer delegates to the intensely side-effectfulsetConfigurationFields
helper 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_WORKSPACE
environment variable once again works properly for selecting the active workspace when using thecloud
block with workspacetags
.