Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature add controller version to version command #1527

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 39 additions & 0 deletions pkg/kudoctl/cmd/version.go
Original file line number Diff line number Diff line change
@@ -1,10 +1,14 @@
package cmd

import (
"errors"
"fmt"

"github.com/spf13/cobra"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"

"github.com/kudobuilder/kudo/pkg/kudoctl/clog"
"github.com/kudobuilder/kudo/pkg/kudoctl/kube"
"github.com/kudobuilder/kudo/pkg/version"
)

Expand All @@ -30,5 +34,40 @@ func newVersionCmd() *cobra.Command {
func VersionCmd(cmd *cobra.Command, args []string) error {
kudoVersion := version.Get()
fmt.Printf("KUDO Version: %s\n", fmt.Sprintf("%#v", kudoVersion))

// Print the Controller Version
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary comment, it's clear from the code what this does.

controllerVersion, err := GetControllerVersion()
if err != nil {
fmt.Printf("KUDO Controller Version: %s\n", controllerVersion)
} else {
fmt.Printf("KUDO Controller Version: %#v\n", err)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's return an error instead.

Suggested change
fmt.Printf("KUDO Controller Version: %#v\n", err)
return fmt.Errorf("failed to retrieve KUDO controller version: %v\n", err)

}

return nil
}

// GetControllerVersion
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// GetControllerVersion
// GetControllerVersion retrieves the version of the controller manager

func GetControllerVersion() (string, error) {

controllerVersion := ""

client, err := kube.GetKubeClient(Settings.KubeConfig)
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>")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please return an error message that includes the underlying error. Also let's stay consistent with other error messages.

Suggested change
return "", errors.New("<Failed to acquire kudo client>")
return "", fmt.Errorf("failed to acquire Kubernetes client: %v", err)

}

statefulsets, err := client.KubeClient.AppsV1().StatefulSets("").List(metav1.ListOptions{LabelSelector: "app=kudo-manager"})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also use the existing function to pull the labelselectors:
e.g.

selector := manager.GenerateLabels().AsSelector()

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>")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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
}
Comment on lines +68 to +70
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, and we also have to handle the case that KUDO isn't deployed on the cluster yet.


return controllerVersion, nil
}