Add a way to unset global config values#1086
Add a way to unset global config values#1086nkubala merged 4 commits intoGoogleContainerTools:masterfrom
Conversation
nkubala
force-pushed
the
unset_global_config
branch
from
d2b30c7 to
374e997
Compare
Codecov Report
@@ Coverage Diff @@
## master #1086 +/- ##
==========================================
- Coverage 44.09% 42.93% -1.16%
==========================================
Files 84 89 +5
Lines 3667 3915 +248
==========================================
+ Hits 1617 1681 +64
- Misses 1895 2066 +171
- Partials 155 168 +13
Continue to review full report at Codecov.
|
nkubala
force-pushed
the
unset_global_config
branch
from
374e997 to
11d789d
Compare
integration/run_test.go
Outdated
| } | ||
|
|
||
| // unset the values and ensure they're not present in the config | ||
| unsetArgs := []string{"config", "unset", test.key} |
There was a problem hiding this comment.
this way of testing a functionality as a "step" of an integration test and depending on the output format is brittle and slow to give feedback. Do you mind writing a simple unit test that exercises the config file, sets and unsets the value?
cmd/skaffold/app/cmd/config/set.go
Outdated
|
|
||
| if fieldType != val.Type() { | ||
| return fmt.Errorf("%s is not a valid value for field %s", value, fieldName) | ||
| if IsZero(val) { |
There was a problem hiding this comment.
Are you introducing this to handle empty strings? If that's the case, we can just ignore the empty string case - if user sets "" then the value will be set to "".
This way we don't have to introduce the whole IsZero function which was decided to be abandoned in the reflect package as well (see this comment). For the first branch value == nil check should be sufficient to set with reflect.Zero
There was a problem hiding this comment.
The reason I added this was to make the unset logic generic for all types (i.e. so we can pass nil to setConfigValue without worrying about the type), in case we added values of different types to the global config (e.g. int, bool, etc) - empty string is actually the only case I didn't need IsZero for. However, after thinking about it a little more I think it's fine for now to just pass the empty string when we're unsetting. I'll remove this.
nkubala
force-pushed
the
unset_global_config
branch
from
cc8724c to
144f694
Compare
nkubala
dismissed
balopat’s stale review
I believe I addressed all the requested changes
4 participants
skaffold config unset default-repo