Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Make updateKeyValueStream reject unsupported options #3753
Conversation
mvo5
added some commits
Aug 17, 2017
Some checks were not successful
mvo5
referenced this pull request
Aug 17, 2017
Merged
corecfg: fix proxy.* writing and add integration test #3754
pedronis
approved these changes
Aug 18, 2017
lgtm, but I'm sure about the change to proxy.go, it seems a fix but is not in the description of the PR
| - "http": true, | ||
| - "https": true, | ||
| - "ftp": true, | ||
| + "http_proxy": true, |
zyga
Aug 21, 2017
Contributor
This looks OK upon code inspection. I second @pedronis' comment though. It should be a separate commit with a description of what is going on. It seems this was just a bug all along and we didn't notice because of a bug in the updateKeyValueStream. With that fix this one surfaced.
mvo5
Aug 21, 2017
Collaborator
This is properly fixed and tested in #3754 - maybe we should focus on this one instead than this one here? Unfortunately removing this change will break the tests (because the bugfix in here exposes a bug in the proxy handling)
| + } | ||
| + | ||
| + _, err := corecfg.UpdateKeyValueStream(in, supportedConfigKeys, newConfig) | ||
| + c.Check(err, ErrorMatches, `cannot set unsupported configuration value \"unsupported-options"`) |
| - "http": true, | ||
| - "https": true, | ||
| - "ftp": true, | ||
| + "http_proxy": true, |
zyga
Aug 21, 2017
Contributor
This looks OK upon code inspection. I second @pedronis' comment though. It should be a separate commit with a description of what is going on. It seems this was just a bug all along and we didn't notice because of a bug in the updateKeyValueStream. With that fix this one surfaced.
mvo5
Aug 21, 2017
Collaborator
This is properly fixed and tested in #3754 - maybe we should focus on this one instead than this one here? Unfortunately removing this change will break the tests (because the bugfix in here exposes a bug in the proxy handling)
| + } | ||
| + | ||
| + _, err := corecfg.UpdateKeyValueStream(in, supportedConfigKeys, newConfig) | ||
| + c.Check(err, ErrorMatches, `cannot set unsupported configuration value \"unsupported-options"`) |
codecov-io
commented
Aug 21, 2017
•
Edited 1 time
-
codecov-io
Aug 21, 2017
-
Aug 21, 2017
Codecov Report
@@ Coverage Diff @@
## master #3753 +/- ##
==========================================
+ Coverage 75.83% 75.85% +0.01%
==========================================
Files 399 399
Lines 34516 34517 +1
==========================================
+ Hits 26176 26182 +6
+ Misses 6478 6474 -4
+ Partials 1862 1861 -1
Continue to review full report at Codecov.
|
mvo5 commentedAug 17, 2017
This also improves the tests and superseeds #3746