-
Notifications
You must be signed in to change notification settings - Fork 76
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
Add command-line flags to replace env variables #305
Conversation
use "go test -tags functional -vcd-help" for a list of flags
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.
Two requests inline, LGTM otherwise. Before merging please confirm there are no side-effects of running the test suite with env vars vs. command line flags vs. when you have both by accident.
@@ -309,24 +309,26 @@ func (vcd *TestVCD) Test_ComposeVApp(check *checks.C) { | |||
} | |||
``` | |||
|
|||
# Environment variables | |||
# Environment variables and corresponding flags | |||
|
|||
While running tests, the following environment variables can be used: |
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.
We should state which is higher priority too - env vars or flags.
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.
Environment variables have precedence.
With this request, you have actually found a bug. Going to fix it
govcd/api_vcd_test.go
Outdated
*variable = true | ||
return | ||
} | ||
flag.BoolVar(variable, name, *variable, help) |
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.
This is actually tricky to read, because variable
is used twice in the same line. And it's not clear how can it have the default value at this early point in time too. To clarify, please add a comment about this line and also about this function.
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.
The first variable
is a pointer, which is used to pass to flag.BoolVar
. The second one is its value, which is whatever was set when the variable was declared.
They are Booleans, thus the default value is false
, which is good enough for flag.BoolVar
When both the flag and the corresponding environment variable were set, we would get an error, because the flag was discarded. Now the env variable sets the value for the flag, and they both work, either in isolation or together
TESTING.md
Outdated
* `VCD_TOKEN` : specifies the authorization token to use instead of username/password | ||
(Use `./scripts/get_token.sh` to retrieve one) | ||
|
||
When both the environment variable and the command line option are possible, the environment variable gets evaluated first: is it is set, | ||
also the flag becomes true. |
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.
Distorted sentence.
Same as the equivalent PR in terraform-provider-vcd, this change enables command line flags that
can be used instead of environment variables.
Use "go test -tags functional -vcd-help" for a list of flags