-
Notifications
You must be signed in to change notification settings - Fork 49
pkg/k8sutil: change NewClientset to load kubeconfig from file content, not from path #628
Conversation
abcbe0f
to
50a07b8
Compare
50a07b8
to
d51b7e7
Compare
Rebased to resolve conflicts. |
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.
Some comments.
cli/cmd/cluster-apply.go
Outdated
@@ -125,7 +125,7 @@ func runClusterApply(cmd *cobra.Command, args []string) { | |||
} | |||
|
|||
func verifyCluster(kubeconfigPath string, expectedNodes int) error { | |||
client, err := k8sutil.NewClientset(kubeconfigPath) | |||
client, err := k8sutil.NewClientsetFromFile(kubeconfigPath) |
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.
The commit message is very confusing, can you try to rephrase?
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.
Right. Sorry about that, I wrote complete nonsense. I rephrased now, it should be better :)
d51b7e7
to
e50c13d
Compare
I applied your suggestions, thanks for reviewing @iaguis 🙏 |
63c0256
to
5703bcd
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.
Thanks for the PR @invidian.
Just a small nit, apart from that lgtm.
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.
+1 LGTM
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.
Reviewed the later half of the PR :)
5703bcd
to
2ba8cef
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.
Thanks for the PR @invidian.
Looks great !!
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
This commit renames NewClientset function into NewClientsetFromFile to better describe it's functionality, as function takes file path as an argument to build the clientset object. It also makes room for new NewClientset function, which will take content of kubeconfig file instead of file path, to make it decoupled from dependency on local filesystem. NewClientsetFromFile is only a temporary and should be removed once we switch back to use NewClientset function everywhere. This commit also adds some small documentation to NewClientsetFromFile function to avoid triggering the linter. Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
This commit adds NewClientset exported function, which allows creating Kubernetes Clientset from content of kubeconfig file, which allows to create it from sources different than file on local file system. This is required for being able to execute 'lokoctl health' without having kubeconfig file present in local assets directory, as it can be pulled from other places, e.g. Terraform state. See #608 for more details. Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
In some places we use 'client' to refer to it, in other 'cs', we should be consistent in it. This commit fixes that. The consistency is particulary useful when you grep for code calling the function. Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
NewClientsetFromFile is now deprecated, as we want to gradually switch to loading kubeconfig file from the file content rather than from the file path. This commit preserves existing functionality, but uses new function, which should simplify the change later for loading from the file content. The code duplication here is intentional, to put calls to NewClientset function in place. In next commits, functions parameter types will be changed and calls to iotuil.ReadFile will be removed. This allows to do more step-by-step transition to different data flow. Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
NewClientsetFromFile is now deprecated, as we want to gradually switch to loading kubeconfig file from the file content rather than from the file path. This commit preserves existing functionality, but uses new function, which should simplify the change later for loading from the file content. Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
All uses of this function are now replaced by NewClientset function, so this function is unused and safe to remove. Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
2ba8cef
to
90f3c1a
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
As a first step towards resolving #608, this PR changes
NewClientset
to create Kubernetes Clientset fromkubeconfig
file content, instead of from file path. The clients are changes to manually load the file content, which should simplify changing it to also accept the file content as an argument in the upcoming patches.