Feature add controller version to version command#1527
Feature add controller version to version command#1527sivaramsk wants to merge 4 commits intokudobuilder:mainfrom
Conversation
sivaramsk
requested review from
alenkacz,
gerred,
kensipe,
nfnt and
zen-dog
as code owners
|
I guess this is still a WIP. Not sure how to handle the testing though, guess I can't do a unit testing since I don't have any parameters to pass and not sure how to do an integration testing as I don't see any examples to use for version. If you could give an example I will write an integration test as well. |
nfnt
left a comment
nfnt
left a comment
There was a problem hiding this comment.
This is a great start, thanks for improving the KUDO experience! I agree that this needs integration tests, because there are some subtle things to consider. The controller manager can be configured to run in any namespace, not just kudo-system and we need to make sure that this is covered by the tests. Take a look at init_integration_test.go for similar integration tests around kudo init.
| kudoVersion := version.Get() | ||
| fmt.Printf("KUDO Version: %s\n", fmt.Sprintf("%#v", kudoVersion)) | ||
|
|
||
| // Print the Controller Version |
There was a problem hiding this comment.
Unnecessary comment, it's clear from the code what this does.
| if err != nil { | ||
| fmt.Printf("KUDO Controller Version: %s\n", controllerVersion) | ||
| } else { | ||
| fmt.Printf("KUDO Controller Version: %#v\n", err) |
There was a problem hiding this comment.
Let's return an error instead.
| fmt.Printf("KUDO Controller Version: %#v\n", err) | |
| return fmt.Errorf("failed to retrieve KUDO controller version: %v\n", err) |
| return nil | ||
| } | ||
|
|
||
| // GetControllerVersion |
There was a problem hiding this comment.
| // GetControllerVersion | |
| // GetControllerVersion retrieves the version of the controller manager |
| clog.V(3).Printf("Acquiring kudo client") | ||
| if err != nil { | ||
| clog.V(3).Printf("Failed to acquire kudo client") | ||
| return "", errors.New("<Failed to acquire kudo client>") |
There was a problem hiding this comment.
Please return an error message that includes the underlying error. Also let's stay consistent with other error messages.
| return "", errors.New("<Failed to acquire kudo client>") | |
| return "", fmt.Errorf("failed to acquire Kubernetes client: %v", err) |
| return "", errors.New("<Failed to acquire kudo client>") | ||
| } | ||
|
|
||
| statefulsets, err := client.KubeClient.AppsV1().StatefulSets("").List(metav1.ListOptions{LabelSelector: "app=kudo-manager"}) |
There was a problem hiding this comment.
This needs to be tested because the KUDO manager may be in kudo-system or other namespaces. We need to ensure that this List can cover both cases.
There was a problem hiding this comment.
We should also use the existing function to pull the labelselectors:
e.g.
kudo/pkg/kudoctl/kudoinit/setup/wait.go
Line 40 in 93b8220
| clog.V(3).Printf("List statefulsets and filter kudo-manager") | ||
| if err != nil { | ||
| clog.V(3).Printf("Failed to list kudo-manager statefulset") | ||
| return "", errors.New("<Error: Failed to list kudo-manager statefulset>") |
There was a problem hiding this comment.
| return "", errors.New("<Error: Failed to list kudo-manager statefulset>") | |
| return "", fmt.Errorf("failed to list kudo-manager statefulset: %v", err) |
| for _, d := range statefulsets.Items { | ||
| controllerVersion = d.Spec.Template.Spec.Containers[0].Image | ||
| } |
There was a problem hiding this comment.
There's a lot of assumptions here. Let's be a bit more strict about them. We assume that the StatefulSet runs a single container which is the manager. Let's add checks to double down on these assumptions:
if len(statefulsets.Items) > 1 {
return errors.New("more than 1 KUDO controller manager running")
}
if len(statefulsets.Items[0].Spec.Template.Spec.Containers) != 1 {
return fmt.Errorf("invalid number of containers in statefulset %s/%s", statefulsets.Items[0].Namespace, statefulsets.Items[0].Name)
}
controllerVersion := statefulsets.Items[0].Spec.Containers[0].Image
There was a problem hiding this comment.
Oh, and we also have to handle the case that KUDO isn't deployed on the cluster yet.
|
@sivaramsk thanks for the PR... I hope you are coming back to update based @nfnt feedback. |
runyontr
left a comment
runyontr
left a comment
There was a problem hiding this comment.
It feels like this should be combined with #1505. Being able to read the current version of the KUDO controller is important to identifying when an upgrade is required. These should use the same function capability instead of doing it separately
| return "", errors.New("<Failed to acquire kudo client>") | ||
| } | ||
|
|
||
| statefulsets, err := client.KubeClient.AppsV1().StatefulSets("").List(metav1.ListOptions{LabelSelector: "app=kudo-manager"}) |
There was a problem hiding this comment.
We should also use the existing function to pull the labelselectors:
e.g.
kudo/pkg/kudoctl/kudoinit/setup/wait.go
Line 40 in 93b8220
4 participants
What this PR does / why we need it:
Print the controller version if installed as part of the kudo version command
Fixes #1295