Add acceptance and internal tests for gcs backend, add some test helper functions
#35026
Conversation
SarahFrench
force-pushed
the
add-gcs-test
branch
from
01757ad
to
c0622f8
Compare
SarahFrench
force-pushed
the
add-gcs-test
branch
from
c0622f8
to
ed83722
Compare
…doesn't affect use of ENV-supplied credentials
…rp/terraform PRs
… let checks pass in hashicorp/terraform repo.
SarahFrench
force-pushed
the
add-gcs-test
branch
from
f93c4f7
to
b5abe9f
Compare
|
Just rebased to include the latest gcs PR |
Move `TestBackendEncryptionKeyEmptyConflict` test to internal test file
SarahFrench
force-pushed
the
add-gcs-test
branch
from
5801a6d
to
e31ddb3
Compare
SarahFrench
changed the title
gcs backend, add some test helper functions
| // getWantValue is required because the key input is changed internally in the backend's code | ||
| // This function is a quick way to help us get a want value, but ideally in future the test and | ||
| // the code under test will use a reusable function to avoid logic duplication. | ||
| getWantValue := func(key string) []byte { |
There was a problem hiding this comment.
I see you've made this a closure because you want access to the t variable but you could just make this it's own func with testing.T as a parameter.
edit: ah, you mention this in the comment above
| getWantValue := func(key string) []byte { | ||
| var want []byte | ||
| if key == "" { | ||
| want = nil |
There was a problem hiding this comment.
Could just return nil here since the next conditional is key != ""?
| if key == "" { | ||
| want = nil | ||
| } | ||
| if key != "" { |
There was a problem hiding this comment.
This could just be else or not even be a conditional if we just return early in the conditional above.
| "GOOGLE_CREDENTIALS": credentials, | ||
| }, | ||
| }, | ||
| // Uncomment below for sanity checking |
There was a problem hiding this comment.
It's probably best not to merge in lines of commented out code – if this is a purposeful test it would be better to formulate it in a way that a failing test isn't the desired outcome.
| preCheckEnvironmentVariables(t) | ||
| // Cannot use t.Parallel as t.SetEnv used | ||
|
|
||
| // getWantValue is required because the key input is changed internally in the backend's code |
There was a problem hiding this comment.
I know we are just copying convention used elsewhere in the codebase, but trying to parse "get want value" hurt my brain. We can leave as is, but I suspect the language we wanted to use here was expected instead of want.
| if got := b.stateFile(c.name); got != c.wantStateFile { | ||
| t.Errorf("stateFile(%q) = %q, want %q", c.name, got, c.wantStateFile) | ||
| } | ||
| // TODO - implement a separate function specific to credentials which will iterate through a list of ENV names and return first value found. |
There was a problem hiding this comment.
Is this a note to self to be answered in this PR, or for later?
Historically the
gcsbackend hasn't been touched much. In the past I've added a few features but largely left the existing codebase as it was previously. Recently thegcsbackend was refactored in #34989 to avoid using the old SDK.As part of reviewing that PR I started adding a few acceptance tests in this PR. There's a lot more work to be done but I want to avoid this PR getting too big.
This PR:
gcsbackend set up (//TODO!)TestBackendConfig_encryptionKey- how the config and ENVs for customer-supplied encryption keys are usedTestBackendConfig_kmsKey- how the config and ENVs for customer-managed (KMS) encryption keys are usedTestAccBackendConfig_credentials- empty strings in the backend config don't interrupt use of ENVs for creds.Target Release
1.8.x
Draft CHANGELOG entry
NEW FEATURES | UPGRADE NOTES | ENHANCEMENTS | BUG FIXES | EXPERIMENTS