Repository navigation
Fix breaking-changes test to skip cluster-init when kubeconfigs unavailable - #4811
Conversation
|
Pipeline controller notification For optional jobs, comment |
WalkthroughModified error handling in kubeclient creation to log warnings and continue iterating rather than halting on missing/invalid kubeconfigs. Added conditional guard in test script to execute cluster-init update only when KUBECONFIG_DIR environment variable is set and points to an existing directory. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cmd/cluster-init/cmd/onboard/config/update.go (1)
95-96: Error handling approach looks good for CI resilience.The change from failing immediately to logging and continuing is appropriate for the breaking-changes test scenario where kubeconfigs may be unavailable. The log message clearly identifies the skipped cluster and the reason.
However, consider that if all clusters fail due to missing kubeconfigs, the function still returns success (nil at line 106). This might be the intended behavior for CI environments, but verify this won't mask legitimate configuration issues in production workflows.
Consider adding a summary log after the loop to track how many clusters were successfully processed vs. skipped, which would make debugging easier:
successCount := 0 skipCount := 0 for clusterName, clusterInstall := range clusterInstalls { ctrlClient, kubeClient, config, err := newKubeClients(kubeconfigs, clusterName) clusterInstall.Config = config if err != nil { log.WithField("cluster", clusterName).WithError(err).Warn("Skipping cluster due to missing or invalid kubeconfig") skipCount++ continue } if err := addClusterInstallRuntimeInfo(ctx, clusterInstall, ctrlClient); err != nil { return err } if err := runConfigSteps(ctx, log, true, clusterInstall, ctrlClient, kubeClient); err != nil { return fmt.Errorf("update config for cluster %s: %w", clusterName, err) } successCount++ } log.WithField("processed", successCount).WithField("skipped", skipCount).Info("Cluster update summary")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
cmd/cluster-init/cmd/onboard/config/update.go(1 hunks)test/validate-generation-breaking-changes.sh(2 hunks)
🔇 Additional comments (1)
test/validate-generation-breaking-changes.sh (1)
68-85: Conditional guard implementation is solid.The check correctly uses
${KUBECONFIG_DIR:-}to safely handle unset variables (compatible withset -o nounsetat line 4) and-dto verify the directory exists before attempting cluster-init update. The skip message clearly explains why the step was bypassed.The structure preserves the original error-detection logic (lines 72-82) when cluster-init does run, while gracefully degrading when kubeconfigs are unavailable—aligning well with the resilient error handling added to
update.go.
|
Scheduling required tests: Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deepsm007, Prucek The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@deepsm007: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2c7f1cc
into
openshift:main
/cc @openshift/test-platform