Skip to content
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

Fix sslVerify default=true for grafanaNet route using new syntax #291

Merged
merged 1 commit into from
Aug 16, 2018

Conversation

Dieterbe
Copy link
Contributor

@Dieterbe Dieterbe commented Aug 16, 2018

Master:

config sslVerify property
unspecified false
true true
false false

When the value is not specified it should default to true.
Our code doesn't know the difference between unspecified and explicitly specified to false,
so it assumes the latter, and when unspecified, changes the value from the default (true) back to false.

With patch applied:

config sslVerify property
unspecified true
true true
false false

Notes:

  • this affects all boolean config options that default to true. but sslVerify is the only case of this.
  • the deprecated old syntax does not have this problem
  • SslVerify, SSlVerIfy, SSLVERIFY all work to set the value explicitly (with new syntax)
  • This issue came up briefly at Unmarshal with default values BurntSushi/toml#47
    It's unfortunate to have to patch the toml dependency code, I tried doing it more cleaner using
    something like meta.IsDefined("route", "sslverify") but couldn't get that to work
    as we have a slice of routes, and the IsDefined doesn't seem to support iterating over a slice and finding
    a specific route.

@Dieterbe Dieterbe requested a review from DanCech August 16, 2018 11:26
Master:

| config      | sslVerify property |
| ----------- | ------------------ |
| unspecified | false              |
| true        | true               |
| false       | false              |

When the value is not specified it should default to true.
Our code doesn't know the difference between unspecified and explicitly specified to false,
so it assumes the latter, and when unspecified, changes the value from the default (true) back to false.

With patch applied:

| config      | sslVerify property |
| ----------- | ------------------ |
| unspecified | true               |
| true        | true               |
| false       | false              |

Notes:
* this affects all boolean config options that default to true. but sslVerify is the only case of this.
* the deprecated old syntax does not have this problem
* SslVerify, SSlVerIfy, SSLVERIFY all work to set the value explicitly (with new syntax)
* This issue came up briefly at BurntSushi/toml#47
  It's unfortunate to have to patch the toml dependency code, I tried doing it more cleaner using
  something like `meta.IsDefined("route", "sslverify")` but couldn't get that to work
  as we have a slice of routes, and the IsDefined doesn't seem to support iterating over a slice and finding
  a specific route.
Copy link
Member

@woodsaj woodsaj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

would have been nicer if we just originally named the arg sslSkipVerify or similar so we could have a default value of False. But changing the arg name now would be a pain.

@Dieterbe Dieterbe merged commit 0c3e2c2 into master Aug 16, 2018
Dieterbe added a commit to Dieterbe/toml that referenced this pull request Oct 15, 2018
Dieterbe added a commit that referenced this pull request Oct 15, 2018
originally patch in vendor dir
#291
now via Dieterbe/toml@96f3d82
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants