gsctl: Ensure that certificate paths get updated with config dir migration #63
Conversation
Codecov Report
@@ Coverage Diff @@
## master #63 +/- ##
==========================================
+ Coverage 39.11% 39.56% +0.45%
==========================================
Files 23 23
Lines 854 867 +13
==========================================
+ Hits 334 343 +9
- Misses 478 479 +1
- Partials 42 45 +3
Continue to review full report at Codecov.
|
| if confReadErr != nil { | ||
| t.Error(confReadErr) | ||
| } | ||
| t.Log(string(newConf)) |
There was a problem hiding this comment.
Is this logged to check manually? If so should it be removed?
There was a problem hiding this comment.
It will be output if the user starts tests in verbose mode (which btw Atom with go-plus does by default), so I still see a use for it.
There was a problem hiding this comment.
Cool that's fine then. I just wanted to check. BTW I'm currently trying out VS Code and liking it a lot.
config/config.go
Outdated
| // adapt certificate paths in kubeconfig | ||
| if len(KubeConfigPaths) > 0 { | ||
| // we only adapt the first file found (on purpose) | ||
| if stat, err := os.Stat(KubeConfigPaths[0]); err == nil { |
There was a problem hiding this comment.
I don't like the KubeConfigPaths[0] I'm not sure what this is and makes the code hard to read. I'd also prefer to see a variable as its used multiple times.
There was a problem hiding this comment.
Not sure I understood that as you meant it. Changed it. Better?
|
@rossf7 I see what you mean, but I couldn't come up with a better alternative. What I'm trying to say is: Of all the possible kubectl configuration paths, this is the f***ing single one I want to deal with here. |
config/config.go
Outdated
| // write back | ||
| writeErr := ioutil.WriteFile(theKubeConfigPath, []byte(newConfig), stat.Mode()) | ||
| if writeErr != nil { | ||
| return fmt.Errorf("Could not overwrite kubectl config file. %s", writeErr.Error()) |
There was a problem hiding this comment.
Usually go errors doesn't start with capital letter and terminate with full stop.
There was a problem hiding this comment.
True, I'll change that. Tim would also have demanded that. :)
for golang style consistency
|
Thanks for the reviews! |
config/config.go
Outdated
| @@ -299,7 +299,7 @@ func migrateConfigDir() error { | |||
| // write back | |||
| writeErr := ioutil.WriteFile(theKubeConfigPath, []byte(newConfig), stat.Mode()) | |||
| if writeErr != nil { | |||
| return fmt.Errorf("Could not overwrite kubectl config file. %s", writeErr.Error()) | |||
| return errors.New("could not overwrite kubectl config file") | |||
There was a problem hiding this comment.
I'd say do it like return fmt.Errorf("could not overwrite kubectl config file: %s", writeErr.Error()), unless you don't care about writeErr message.
BTW in almost all cases error variable is called err and reused.
Fixes https://github.com/giantswarm/gsctl/issues/59
In #55 a migration of the configuration directory has been introduced. Unfortunately I forgot to take care of a user's kubectl config file.
This PR here makes the migration also maintain the paths. It does it simply by replacing occurrences of the old path with the new path in the config, not trying to be any more clever than that.