Fix setting env name with a property #568
Conversation
| s.NoError(os.Setenv("TEMPORAL_ENV", "myenv")) | ||
| defer os.Unsetenv("TEMPORAL_ENV") |
There was a problem hiding this comment.
You should be able to replace LookupEnv in the CLI options instead of using actual environment variables that affect everything globally. See TestWorkflow_Execute_EnvVars in commands.workflow_exec_test.go for an example (I saw we unfortunately have others not doing this in tests that should).
There was a problem hiding this comment.
I saw that but I though using a system property "for real" may be a better test than faking it... Are there any side-effects of using a real system property in the test?
There was a problem hiding this comment.
This can damage our ability to do parallel tests in the future. Tests need to be as self-contained as possible. It's mocked out at the highest level, so you're not getting much more using the real env var I don't think.
| @@ -89,6 +89,8 @@ func NewCommandContext(ctx context.Context, options CommandOptions) (*CommandCon | |||
| return cctx, stop, nil | |||
| } | |||
|
|
|||
| const temporalEnv = "TEMPORAL_ENV" | |||
There was a problem hiding this comment.
I don't think there's much value in making this a constant that is used in one place, but meh
What was changed
Ensure that we read the property
TEMPORAL_ENV, which sets then env name,early on, i.e., in
preprocessOptions.Why?
Otherwise it is too late to have an effect...
Checklist
[Bug] TEMPORAL_ENV may not be working #541
Added a system test
3. Any docs updates needed?
No