feat(cluster-version): read version from ClusterKubeconfig label by onuryilmaz · Pull Request #82 · cloudoperators/cloudctl

onuryilmaz · 2026-09-16T10:48:24Z

Summary

Adds label-based version lookup to cluster-version: reads the greenhouse.sap/kubernetes-version label from the ClusterKubeconfig resource on Greenhouse instead of hitting the remote cluster's /version endpoint
Falls back gracefully to the existing live-query path (unauthenticated → authenticated) when the label is absent, the resource is not found, or the Greenhouse connection fails (logged at debug level)
Adds four new optional flags: -g/--greenhouse-cluster-kubeconfig, --greenhouse-cluster-context, -n/--greenhouse-cluster-namespace, --greenhouse-cluster-name

Closes #81

Test plan

When --greenhouse-cluster-namespace and --greenhouse-cluster-name are provided, cluster-version first attempts to read the greenhouse.sap/kubernetes-version label from the ClusterKubeconfig resource on Greenhouse. This is faster and works even when the remote API server is temporarily unavailable. If the label is absent, the resource is not found, or the Greenhouse connection fails, the command falls back to the existing live query path (unauthenticated GET /version → authenticated fallback). New flags on cluster-version: -g, --greenhouse-cluster-kubeconfig path to Greenhouse kubeconfig --greenhouse-cluster-context context in that kubeconfig -n, --greenhouse-cluster-namespace Greenhouse org namespace --greenhouse-cluster-name ClusterKubeconfig resource name Closes #81 Signed-off-by: onuryilmaz <onur.yilmaz@sap.com>

Copilot

🟡 Changes recommended

Unresolved issues remain in fallback handling, timeout behavior, flag binding, version normalization, and kubeconfig validation.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

This pull request adds Greenhouse label-based Kubernetes version lookup to cluster-version, with fallback to live cluster queries.

Changes:

Added Greenhouse flags and label lookup logic.
Preserved live-query fallback behavior.
Added tests and an indirect dependency update.

File summaries

File	Description
`go.mod`	Adds a transitive dependency.
`cmd/cluster-version.go`	Implements label lookup, fallback behavior, and flags.
`cmd/cluster-version_test.go`	Tests label lookup and flag registration.

Review details

Suppressed comments (3)

cmd/cluster-version.go:141

The label value is returned verbatim, while the existing live-query path strips a leading v, prerelease suffix, and build metadata before producing ClusterVersionResult. Because this label is sourced from status.kubernetesVersion, values such as v1.29.3 produce different output from the live path, violating the unchanged output contract. Apply the same normalization before assigning clusterVersion.

			clusterVersion = labelVer

cmd/cluster-version.go:85

An explicitly empty --greenhouse-cluster-kubeconfig is not rejected here, unlike the equivalent validation in cmd/sync.go:122-128. resolveKubeconfig then returns an empty path and configWithContext silently loads $KUBECONFIG or ~/.kube/config, which can make the label lookup query an unintended cluster. Add the same explicit-empty guard before reading the other Greenhouse flags.

	cvGreenhouseKubeconfig = resolveKubeconfig("greenhouse-cluster-kubeconfig", viper.GetString("greenhouse-cluster-kubeconfig"))
	cvGreenhouseContext = viper.GetString("greenhouse-cluster-context")
	cvGreenhouseNamespace = viper.GetString("greenhouse-cluster-namespace")
	cvGreenhouseClusterName = viper.GetString("greenhouse-cluster-name")

cmd/cluster-version.go:326

The repository's README documents the cluster-version flags at README.md:147-151, but it is not updated for these four new flags or the label-first behavior and still describes only live API queries. Update that command section so users can discover and configure the new lookup path.

	clusterVersionCmd.Flags().StringVarP(&cvGreenhouseKubeconfig, "greenhouse-cluster-kubeconfig", "g", clientcmd.RecommendedHomeFile, "Path to the Greenhouse cluster kubeconfig (for label-based version lookup)")
	clusterVersionCmd.Flags().StringVar(&cvGreenhouseContext, "greenhouse-cluster-context", "", "Context to use from the Greenhouse kubeconfig")
	clusterVersionCmd.Flags().StringVarP(&cvGreenhouseNamespace, "greenhouse-cluster-namespace", "n", "", "Greenhouse organization namespace")
	clusterVersionCmd.Flags().StringVar(&cvGreenhouseClusterName, "greenhouse-cluster-name", "", "ClusterKubeconfig resource name in Greenhouse to read the version label from")

Files reviewed: 3/3 changed files
Comments generated: 3
Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

…use kubeconfig - Apply the same normalizeVersion() stripping (leading v, prerelease, build metadata) to the label path so output is identical to the live query path regardless of how the Greenhouse controller formats the version string (e.g. "v1.31.4+k3s1" → "1.31.4") - Reject an explicitly empty --greenhouse-cluster-kubeconfig, matching the existing guard in sync.go to prevent silent fallback to an unintended kubeconfig Signed-off-by: onuryilmaz <onur.yilmaz@sap.com>

Signed-off-by: onuryilmaz <onur.yilmaz@sap.com>

- Use cmd.Flags().GetString() for Greenhouse flags instead of viper to avoid key collisions with sync.go's identically-named viper bindings - Give the label lookup its own context with half the total timeout so the live-query fallback always has a meaningful deadline if Greenhouse is slow to respond - Propagate non-NotFound errors from versionLabelFromClient via client.IgnoreNotFound so RBAC/network/timeout failures are observable at debug level rather than silently falling through Signed-off-by: onuryilmaz <onur.yilmaz@sap.com>

Copilot

🟡 Changes recommended

The explicit Greenhouse kubeconfig flag can be ignored, and documentation and command-level coverage remain incomplete.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (3)

Previously missed (1) — in code that hasn't changed since the last review.

cmd/cluster-version.go:343

These new public flags are not added to the README's cluster-version flags section (README.md:138-151), even though the README promises that every flag is configurable and currently enumerates the command flags. Without documenting these inputs, users cannot discover the label lookup path or its required namespace/name values; update the command documentation and example.

cmd/cluster-version.go:91

GetString on a Cobra flag does not consume values loaded by Viper. Consequently, setting these new flags through CLOUDCTL_GREENHOUSE_CLUSTER_CONTEXT, CLOUDCTL_GREENHOUSE_CLUSTER_NAMESPACE, CLOUDCTL_GREENHOUSE_CLUSTER_NAME, or the config file leaves the corresponding values at their defaults, so the label lookup is skipped. This violates the repository's documented all-flags configuration contract (README.md:88); use the direct pflag value only when the flag was changed and otherwise read a command-safe Viper/config value.

	cvGreenhouseContext, _ = cmd.Flags().GetString("greenhouse-cluster-context")
	cvGreenhouseNamespace, _ = cmd.Flags().GetString("greenhouse-cluster-namespace")
	cvGreenhouseClusterName, _ = cmd.Flags().GetString("greenhouse-cluster-name")

cmd/cluster-version.go:146

The new tests exercise versionLabelFromClient in isolation, but none executes this command-level branch. A regression in the flag gating, the call to getVersionFromLabel, normalization, or the requirement that a label hit avoid the remote /version request would still pass all of these tests. Add a command-level test with controllable Greenhouse and remote endpoints covering both label hit and fallback.

	if cvGreenhouseNamespace != "" && cvGreenhouseClusterName != "" {
		labelCtx, labelCancel := context.WithTimeout(cmd.Context(), timeout/2)
		labelVer, labelErr := getVersionFromLabel(labelCtx, cvGreenhouseKubeconfig, cvGreenhouseContext, cvGreenhouseNamespace, cvGreenhouseClusterName)

Files reviewed: 3/3 changed files
Comments generated: 1
Review effort level: Lite

…ig resolution resolveKubeconfig internally calls viper.IsSet which can be polluted by sync.go's identical Viper key binding. Inline the same KUBECONFIG env-var fallback logic using cmd.Flags().Changed() so an explicit -g always wins regardless of what sync has bound to Viper. Signed-off-by: onuryilmaz <onur.yilmaz@sap.com>

Copilot

🟡 Changes recommended

Configuration precedence and timeout handling issues remain unresolved, along with testing and documentation nits.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (2)

cmd/cluster-version.go:149

Because this sub-context is derived from cmd.Context() instead of the overall ctx, a failed Greenhouse lookup can consume timeout/2 and the live-query fallback can then consume a full timeout, exceeding the documented command deadline (for example, up to 15 seconds with --timeout 10s). Derive the label lookup from ctx so the fallback gets only the remaining portion of the single deadline.

		labelCtx, labelCancel := context.WithTimeout(cmd.Context(), timeout/2)

cmd/cluster-version.go:347

These public flags are missing from the checked-in README.md cluster-version flag list (README.md:147-150), even though that reference documents the command's flags and configuration sources. Users following the command reference will not discover the label lookup or the namespace/name options needed to activate it; update the command reference with these flags and their defaults/precedence.

	clusterVersionCmd.Flags().StringVarP(&cvGreenhouseKubeconfig, "greenhouse-cluster-kubeconfig", "g", clientcmd.RecommendedHomeFile, "Path to the Greenhouse cluster kubeconfig (for label-based version lookup)")
	clusterVersionCmd.Flags().StringVar(&cvGreenhouseContext, "greenhouse-cluster-context", "", "Context to use from the Greenhouse kubeconfig")
	clusterVersionCmd.Flags().StringVarP(&cvGreenhouseNamespace, "greenhouse-cluster-namespace", "n", "", "Greenhouse organization namespace")
	clusterVersionCmd.Flags().StringVar(&cvGreenhouseClusterName, "greenhouse-cluster-name", "", "ClusterKubeconfig resource name in Greenhouse to read the version label from")

Files reviewed: 3/3 changed files
Comments generated: 2
Review effort level: Lite

…l tests - Bind Greenhouse flags under cv.* viper keys so CLOUDCTL_CV_* env vars and .cloudctl.yaml config values work without colliding with sync.go's identically-named bindings; resolveKubeconfig is called with the cv.* key so KUBECONFIG env fallback still applies - Add clusterVersionLabelLookup function variable for test injection - Add two command-level integration tests: one verifies the label path short-circuits the live /version call; the other verifies a lookup error triggers the live-query fallback Signed-off-by: onuryilmaz <onur.yilmaz@sap.com>

Copilot

🟡 Changes recommended

The documented CLOUDCTL_CV_* environment variables are not resolved because dotted Viper keys are not mapped correctly.

Get a fresh assessment by requesting another Copilot review.

Review details

Files reviewed: 3/3 changed files
Comments generated: 1
Review effort level: Balanced

The dot in cv.* viper keys was not handled by the global SetEnvKeyReplacer("-","_"), so CLOUDCTL_CV_GREENHOUSE_CLUSTER_NAMESPACE was silently ignored. Switching to cv-* (hyphen) means the replacer translates the env var name correctly. Signed-off-by: onuryilmaz <onur.yilmaz@sap.com>

onuryilmaz · 2026-09-17T08:48:15Z

Fixed in 58ec404 by switching from the cv.* dot-separated prefix to cv-* (hyphen). The existing SetEnvKeyReplacer("-", "_") in setupConfig now correctly maps CLOUDCTL_CV_GREENHOUSE_CLUSTER_NAMESPACE (and the other three CLOUDCTL_CV_GREENHOUSE_CLUSTER_* env vars) to their respective viper keys.

Copilot

🔵 Needs a closer look

Partial Greenhouse selector input currently causes a silent live-query fallback.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

cmd/cluster-version.go:146

Supplying only one of these two selector flags silently skips the requested Greenhouse lookup and performs a live query instead. This can return a version for the local context while masking a missing namespace/name. Validate that the namespace and cluster name are either both set or both absent before entering the fallback logic.

Files reviewed: 3/3 changed files
Comments generated: 0 new
Review effort level: Balanced

Copilot AI lite review requested due to automatic review settings September 16, 2026 10:48

onuryilmaz requested review from a team as code owners September 16, 2026 10:48

Copilot started reviewing on behalf of onuryilmaz September 16, 2026 10:49 View session

Copilot AI reviewed Sep 16, 2026

View reviewed changes

Comment thread cmd/cluster-version.go Outdated

Comment thread cmd/cluster-version.go Outdated

Comment thread cmd/cluster-version.go

onuryilmaz added 3 commits September 16, 2026 12:56

fix(cluster-version): address errcheck lint findings in test

de4655b

Signed-off-by: onuryilmaz <onur.yilmaz@sap.com>

onuryilmaz requested a lite review from Copilot September 16, 2026 14:11

Copilot started reviewing on behalf of onuryilmaz September 16, 2026 14:11 View session

Copilot AI reviewed Sep 16, 2026

View reviewed changes

Comment thread cmd/cluster-version.go Outdated

onuryilmaz requested a lite review from Copilot September 17, 2026 08:06

Copilot started reviewing on behalf of onuryilmaz September 17, 2026 08:07 View session

Copilot AI reviewed Sep 17, 2026

View reviewed changes

Comment thread cmd/cluster-version.go Outdated

Comment thread cmd/cluster-version.go Outdated

onuryilmaz requested a balanced review from Copilot September 17, 2026 08:21

Copilot started reviewing on behalf of onuryilmaz September 17, 2026 08:21 View session

Copilot AI reviewed Sep 17, 2026

View reviewed changes

Comment thread cmd/cluster-version.go Outdated

onuryilmaz requested a balanced review from Copilot September 17, 2026 09:59

Copilot started reviewing on behalf of onuryilmaz September 17, 2026 10:00 View session

Copilot AI reviewed Sep 17, 2026

View reviewed changes

onuryilmaz merged commit c122c04 into main Sep 17, 2026
8 checks passed

Navigation Menu

feat(cluster-version): read version from ClusterKubeconfig label - #82

feat(cluster-version): read version from ClusterKubeconfig label#82
onuryilmaz merged 7 commits into
mainfrom
feat/cluster-version-label-lookup

onuryilmaz commented Sep 16, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

onuryilmaz commented Sep 17, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

onuryilmaz commented Sep 16, 2026

Summary

Test plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (3)

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (3)

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (2)

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

onuryilmaz commented Sep 17, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Suppressed comments (1)

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants