-
Notifications
You must be signed in to change notification settings - Fork 631
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
mustParseBool should return false for empty string #749
Conversation
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.
LGTM works as described.
@deekoder don't merge this yet, there is a small change that I wish to do this PR. |
I.e, ENABLE_HTTPS environment variable is not set `mustParseBool(os.Getenv("ENABLE_HTTPS"))` should be `false`
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.
Added comments to separate actual fix and extraneous changes.
@@ -124,7 +124,7 @@ func testMakeBucketError() { | |||
mustParseBool(os.Getenv(enableHTTPS)), | |||
) | |||
if err != nil { | |||
log.Fatalf("Error:", err) | |||
log.Fatalf("Error: %s", err) |
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 was spotted by my editor.
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.
Oops yes..
@@ -3917,7 +3917,7 @@ func testFunctionalV2() { | |||
} | |||
} | |||
|
|||
// Convert string to bool and always return true if any error | |||
// Convert string to bool and always return false if any error |
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.
minor nit fixed, not originally part of this PR
@deekoder This PR can now be merged subject to reviewers' approval. |
I.e, ENABLE_HTTPS environment variable is not set
mustParseBool(os.Getenv("ENABLE_HTTPS"))
should befalse
Fixes #751