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.

Sign up for GitHub

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

Jump to bottom

Feature add controller version to version command #1527

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Feature add controller version to version command #1527

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
deaf3e4
Add function to print controller version and print it.
sivaramsk Apr 25, 2020
4456fab
Merge branch 'master' into feature-add-controller-version-to-version-…
sivaramsk Apr 28, 2020
551fd42
Merge branch 'master' into feature-add-controller-version-to-version-…
sivaramsk May 6, 2020
614d728
Add controller version to the kudo version command.
sivaramsk May 15, 2020
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
View file
Open in desktop
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.

kudo/pkg/kudoctl/kudoinit/setup/wait.go

Line 40 in 93b8220

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
}