-
Notifications
You must be signed in to change notification settings - Fork 49
Conversation
45afd6f
to
6127d0b
Compare
6127d0b
to
8b9e1df
Compare
bb3c326
to
9344de1
Compare
9344de1
to
e9fa3d5
Compare
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 🎉
e9fa3d5
to
2b4b98a
Compare
@ipochi I had to rebase, one more LGTM? |
So all subcommands use the same function to determine which components should be used. Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
As it does not add any value, as we create a slice of objects anyway instead of specifying them by hand. Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
To soften the dependency on log.Entry and to separate the functionality from CLI code. This also makes code easier to move around and avoids hiding function complexity. Part of #630 Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
So they are easier to trace and debug. Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
To soften the dependency on log.Entry and to separate the functionality from CLI code. This also makes code easier to move around and avoids hiding function complexity. Part of #630 Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
2b4b98a
to
a4ddc0e
Compare
CI was not triggering, so I had to re-push |
a4ddc0e
to
8724400
Compare
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.
Small nit.
@@ -187,7 +187,10 @@ func readKubeconfigFromTerraformState(contextLogger *log.Entry) ([]byte, error) | |||
contextLogger.Warn("Kubeconfig file not found in assets directory, pulling kubeconfig from " + | |||
"Terraform state, this might be slow. Run 'lokoctl cluster apply' to fix it.") | |||
|
|||
c, err := initialize(contextLogger) | |||
// TODO: Add Terraform verbose support back. |
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.
I understand this means you can't configure verboseness when reading the kubeconfig from the terraform state.
In that case I think I'm fine with this.
@@ -111,7 +111,11 @@ func clusterApply(contextLogger *log.Entry, options clusterApplyOptions) error { | |||
|
|||
fmt.Printf("\nYour configurations are stored in %s\n", c.assetDir) | |||
|
|||
kubeconfig, err := getKubeconfig(contextLogger, c.lokomotiveConfig, true) | |||
kg := kubeconfigGetter{ |
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.
I'm not super convinced by this getter (especially regarding naming) but I think this is an improvement over the current state so I'm not blocking this.
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.
Yeah, once moved to separate package, we need to continue refactoring...
8724400
to
3455bd0
Compare
To soften the dependency on log.Entry and to separate the functionality from CLI code. This also makes code easier to move around and avoids hiding function complexity. Part of #630 Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io> Co-authored-by: knrt10 <kautilya@kinvolk.io>
This should allow better structure for the code and should make easier to move this code to some other package. Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
Pass required information via options struct instead. Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
Use options struct instead. Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io> Co-authored-by: knrt10 <kautilya@kinvolk.io>
Use config struct instead. Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
To add it a name and also to allow adding more information there, to separate this code from CLI code. Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
Pass explicity kubeconfig path via options instead. Also fixes #1094. Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
3455bd0
to
2c86b58
Compare
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.
looks ok
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
Thanks for merging! |
Includes commits from #1015Part of #603.