chore: Fail fast when migration test is detecting TF_CLI_CONFIG_FILE #2266
Conversation
internal/testutil/mig/provider.go
Outdated
|
|
||
| func validateConflictingEnvVars() { | ||
| if os.Getenv("TF_CLI_CONFIG_FILE") != "" { | ||
| log.Fatal("found `TF_CLI_CONFIG_FILE` in env-var when running migration tests, this might override the terraform provider for MONGODB_ATLAS_LAST_VERSION and instead use local latest build!") |
There was a problem hiding this comment.
Would have liked to use tb.Fatal instead of log.Fatal but I think there is a possibility of versionConstraint being called without checkLastVersion, therefore, I had to use log.Fatal
There was a problem hiding this comment.
sorry, not sure I understand this, can you elaborate or explain in a different way? We shouldn't use log.Fatal in tests but tb.Fatal
There was a problem hiding this comment.
I was afraid of a case where some migration tests would call ExternalProvidersWithAWS and ExternalProviders which call versionConstraint and the migration test would not be skipped.
However, looking at the code, everywhere those are used we expect to see mig.PreCheck or mig.SkipIfVersionBelow used which would check the TF_CLI_CONFIG_FILE env-var and skip the test
Changed in 11ecbc6
| @@ -11,6 +11,7 @@ import ( | |||
|
|
|||
| func SkipIfVersionBelow(tb testing.TB, minVersion string) { | |||
| tb.Helper() | |||
| validateConflictingEnvVars(tb) | |||
There was a problem hiding this comment.
do we need to call it here? I assume that checkLastVersion will be called at some point in a mig test
There was a problem hiding this comment.
I found some examples where only acc pre checks are used, e.g., here.
There was a problem hiding this comment.
oh i see, so we have some inconsistencies as we should call checkLastVersion before calling versionConstraint, but it's not happening in the example you're mentioning.
mig.ExternalProviders calls versionConstraint but checkLastVersion is not called because the test is missing a mig precheck
There was a problem hiding this comment.
all mig tests should call a mig precheck but not sure if it's easy to enforce
There was a problem hiding this comment.
Yes, it would be nice if we could enforce it.
For now, I pefer keeping the validateConflictingEnvVars in the SkipIfVersionBelow 👍
Description
Using TF_CLI_CONFIG_FILE might cause your local "latest" build version of TF provider to be used instead of latest released version (e.g., 1.16.0)
Link to any related issue(s): CLOUDP-248697
Type of change:
Required Checklist:
Further comments