This repository has been archived by the owner on Jan 8, 2024. It is now read-only.
Inspecting boolean env var values. #1699
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Previously, we were taking any value for boolean env vars to mean "true". If someone specified `WAYPOINT_SERVER_TLS=false`, we would take that to mean true. This fixes that. Note on the implementation - I hate to be the person to introduce a generic "util" package. If anyone has any better ideas for where to put this little function, or feels like we should just strconv in-line every time, I am open to those ideas.
catsby
suggested changes
Jun 23, 2021
internal/util/env.go
Outdated
|
|
||
| // Extracts a boolean from an env var. Falls back to the default if | ||
| // The key is unset or not a valid boolean. | ||
| func GetEnvBool(key string, defaultValue bool) (value bool) { |
There was a problem hiding this comment.
Generally avoid named returns unless we're returning multiple of the same type, and naming them serves as documentation.
catsby
reviewed
Jun 23, 2021
.changelog/1699.txt
Outdated
| @@ -0,0 +1,4 @@ | |||
| ```release-note:improvement | |||
| core: Inspect values of enviromment variables meant to be booleans | |||
There was a problem hiding this comment.
Suggested change
| core: Inspect values of enviromment variables meant to be booleans | |
| core: Correct parsing of boolean of some environment variables |
krantzinator
approved these changes
Jun 23, 2021
briancain
suggested changes
Jun 23, 2021
| connecting to the server. | ||
| - `WAYPOINT_SERVER_TLS_SKIP_VERIFY`. Current must be set to `1` to disable TLS verification | ||
| - `WAYPOINT_SERVER_TLS_SKIP_VERIFY`. Current must be set to truthy value (e.g. "1") to disable TLS verification |
There was a problem hiding this comment.
Suggested change
| - `WAYPOINT_SERVER_TLS_SKIP_VERIFY`. Current must be set to truthy value (e.g. "1") to disable TLS verification | |
| - `WAYPOINT_SERVER_TLS_SKIP_VERIFY`. Current must be set to a truthy value (e.g. "1") to disable TLS verification |
krantzinator
approved these changes
Jun 23, 2021
evanphx
suggested changes
Jun 23, 2021
internal/env/env.go
Outdated
| if envVal == "" { | ||
| return defaultValue | ||
| } | ||
| value, err := strconv.ParseBool(envVal) |
There was a problem hiding this comment.
Let's convert envVal to lower before passing it in just to support 🧽 👖 case. Dealers choice if we also support yes / no.
mitchellh
suggested changes
Jun 23, 2021
And thereby conforming to our style guide.
mitchellh
approved these changes
Jun 25, 2021
|
Btw, this can probably be backported if you want. |
briancain
approved these changes
Jun 25, 2021
evanphx
approved these changes
Jun 25, 2021
catsby
approved these changes
Jun 30, 2021
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously, we were taking any value for boolean env vars to mean "true". If someone specified
WAYPOINT_SERVER_TLS=false, we would take that to mean true. This fixes that.Note on the implementation - I hate to be the person to introduce a generic "util" package. If anyone has any better ideas for where to put this little function, or feels like we should just strconv in-line every time, I am open to those ideas.