CNTRLPLANE-596: Add --kubeconfig flag to HyperShift and HCP CLI - #8402
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
openshift-ci-robot
added
the
jira/valid-reference
|
@hypershift-jira-solve-ci[bot]: This pull request references CNTRLPLANE-596 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds explicit kubeconfig-path support across CLI, command handlers, and utilities. New helpers Sequence Diagram(s)sequenceDiagram
participant User as CLI (user)
participant Cmd as Command logic (core/aws/agent/none)
participant Util as cmd/util (GetClientWithKubeconfig / GetConfigWithKubeconfig)
participant K8s as Kubernetes API (apiserver)
User->>Cmd: invoke command (--kubeconfig optional)
Cmd->>Util: GetClientWithKubeconfig(kubeconfigPath)
alt kubeconfigPath != ""
Util->>Util: clientcmd.BuildConfigFromFlags("", kubeconfigPath)
Note right of Util: wrap errors with kubeconfig path context
else kubeconfigPath == ""
Util->>Util: controller-runtime GetConfig() / env resolution
end
Util->>K8s: create controller-runtime client using config
Cmd->>K8s: API calls (Get Secret / Read Nodes / Validate resources)
K8s-->>Cmd: Secret / Node / Resource responses
Cmd-->>User: result (validation / create / destroy)
Suggested reviewers
🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
area/cli
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #8402 +/- ##
==========================================
+ Coverage 42.50% 42.60% +0.09%
==========================================
Files 768 768
Lines 95272 95322 +50
==========================================
+ Hits 40498 40609 +111
+ Misses 51971 51903 -68
- Partials 2803 2810 +7
... and 3 files with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/cluster/core/create.go (1)
651-653:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid wrapping a nil error when no nodes are returned
On Line 652,
%wwrapserr, buterris nil at that point. This produces a misleading message and drops useful context.🛠️ Suggested fix
if len(nodes.Items) < 1 { - return "", fmt.Errorf("no node objects found: %w", err) + return "", fmt.Errorf("no node objects found") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/cluster/core/create.go` around lines 651 - 653, The error return in the check for nodes.Items incorrectly uses fmt.Errorf("no node objects found: %w", err) while err is nil; update the return in the nodes length check to not wrap a nil error — e.g. return "", fmt.Errorf("no node objects found") — or, if there is an actual underlying error to surface, use that concrete error variable instead of err; adjust the code around the nodes/Items check in create.go (the block referencing nodes.Items and err) accordingly.
🧹 Nitpick comments (1)
cmd/cluster/aws/destroy_test.go (1)
17-39: ⚡ Quick winAdd one kubeconfig-focused test case to validate the new parameter behavior.
Line 44 now passes kubeconfig, but current cases don’t actually assert any kubeconfig-dependent path. Add a case with
CredentialSecretNameset and an invalid kubeconfig path (or valid temp kubeconfig) to verify the new argument is functionally covered.Also applies to: 44-44
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/cluster/aws/destroy_test.go` around lines 17 - 39, Add a new test case in the table in cmd/cluster/aws/destroy_test.go that covers the kubeconfig path handling by setting DestroyOptions.CredentialSecretName to a non-empty value and providing a kubeconfig path (either an invalid path expecting expectError=true or a created temp kubeconfig file expecting expectError=false); reference the existing test map entries that use core.DestroyOptions, core.AWSPlatformDestroyOptions and awsutil.AWSCredentialsOptions so the new case exercises the code path that reads/validates kubeconfig (use a temp file creation helper if you need a valid kubeconfig) and set expectError to match the intended behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/util/client_test.go`:
- Around line 49-51: The table-driven test case named "When kubeconfig path is
empty, it should fall back to default config resolution" currently allows err !=
nil to pass silently; update the test in cmd/util/client_test.go so that for the
empty kubeconfigPath case you assert no error is returned (err == nil) and
validate the returned client/config is non-nil (or matches the expected
default-resolution behavior) instead of allowing the branch that accepts any
error; likewise tighten the other cases around lines 81-91 to explicitly assert
success or failure as expected for each test entry (use the test case's name
string to identify each case and replace the permissive err!=nil branch with
explicit assertions).
- Around line 127-129: The test uses t.Setenv("FAKE_CLIENT","true") only when
tc.fakeClient is true, which can leak environment state across subtests; inside
the subtest for the test function (the loop handling tc), ensure FAKE_CLIENT is
explicitly set or cleared for every case by calling
t.Setenv("FAKE_CLIENT","true") when tc.fakeClient is true and
t.Setenv("FAKE_CLIENT","") or t.Setenv("FAKE_CLIENT","false") (or use
os.Unsetenv via t.Setenv with empty string) when tc.fakeClient is false so each
subtest (the code surrounding the if tc.fakeClient block) runs with an isolated
FAKE_CLIENT value.
In `@support/util/clientset_test.go`:
- Around line 74-81: The test currently uses "else if err == nil" which allows a
failing case (expectError==false but err!=nil) to slip through; in the test for
the table case (tc.expectError / tc.errorContains) replace the "else if err ==
nil" branch with an unconditional else that asserts err is nil and that the
returned kube client (kc) is not nil (and optionally assert no unexpected error
message), i.e., when tc.expectError is false assert err == nil and
g.Expect(kc).ToNot(BeNil()) so failures in success paths are caught.
---
Outside diff comments:
In `@cmd/cluster/core/create.go`:
- Around line 651-653: The error return in the check for nodes.Items incorrectly
uses fmt.Errorf("no node objects found: %w", err) while err is nil; update the
return in the nodes length check to not wrap a nil error — e.g. return "",
fmt.Errorf("no node objects found") — or, if there is an actual underlying error
to surface, use that concrete error variable instead of err; adjust the code
around the nodes/Items check in create.go (the block referencing nodes.Items and
err) accordingly.
---
Nitpick comments:
In `@cmd/cluster/aws/destroy_test.go`:
- Around line 17-39: Add a new test case in the table in
cmd/cluster/aws/destroy_test.go that covers the kubeconfig path handling by
setting DestroyOptions.CredentialSecretName to a non-empty value and providing a
kubeconfig path (either an invalid path expecting expectError=true or a created
temp kubeconfig file expecting expectError=false); reference the existing test
map entries that use core.DestroyOptions, core.AWSPlatformDestroyOptions and
awsutil.AWSCredentialsOptions so the new case exercises the code path that
reads/validates kubeconfig (use a temp file creation helper if you need a valid
kubeconfig) and set expectError to match the intended behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 547a30e3-f175-4cf3-bb9b-49bfdf07969a
📒 Files selected for processing (17)
cmd/cluster/agent/create.gocmd/cluster/aws/create.gocmd/cluster/aws/create_test.gocmd/cluster/aws/destroy.gocmd/cluster/aws/destroy_test.gocmd/cluster/cluster.gocmd/cluster/core/create.gocmd/cluster/core/destroy.gocmd/cluster/kubevirt/create.gocmd/cluster/none/create.gocmd/util/client.gocmd/util/client_test.goproduct-cli/cmd/cluster/aws/destroy.goproduct-cli/cmd/cluster/cluster.goproduct-cli/cmd/cluster/cluster_test.gosupport/util/clientset_test.gosupport/util/util.go
c9eebd3 to
dfb18fa
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/util/client.go`:
- Line 23: The KubeconfigFlagHelp string and GetConfigWithKubeconfig behavior
are inconsistent: the help text omits the in-cluster fallback while
GetConfigWithKubeconfig("") delegates to cr.GetConfig() which uses KUBECONFIG →
in-cluster → ~/.kube/config; either update the KubeconfigFlagHelp constant to
explicitly list the in-cluster fallback (e.g., "KUBECONFIG env var → in-cluster
config → ~/.kube/config") or change GetConfigWithKubeconfig (the function that
handles empty kubeconfig paths) to explicitly load kubeconfig using clientcmd
loading rules that match the advertised precedence (use
clientcmd.NewDefaultClientConfigLoadingRules or build a LoadingRules that
prefers KUBECONFIG then ~/.kube/config and avoid cr.GetConfig() in the
empty-path branch); apply the same change/wording to any uses in
support/util/util.go to keep behavior and docs consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 377f8e67-0ad5-476f-88bc-0aad19d4daf9
📒 Files selected for processing (12)
cmd/cluster/aws/create.gocmd/cluster/aws/create_test.gocmd/cluster/aws/destroy.gocmd/cluster/aws/destroy_test.gocmd/cluster/cluster.gocmd/cluster/core/create.gocmd/util/client.gocmd/util/client_test.goproduct-cli/cmd/cluster/aws/destroy.goproduct-cli/cmd/cluster/cluster.gosupport/util/clientset_test.gosupport/util/util.go
✅ Files skipped from review due to trivial changes (2)
- product-cli/cmd/cluster/cluster.go
- cmd/cluster/aws/create_test.go
🚧 Files skipped from review as they are similar to previous changes (7)
- cmd/cluster/aws/destroy_test.go
- cmd/util/client_test.go
- support/util/clientset_test.go
- cmd/cluster/cluster.go
- cmd/cluster/aws/destroy.go
- support/util/util.go
- product-cli/cmd/cluster/aws/destroy.go
dfb18fa to
bf93e3b
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/cluster/core/create.go (1)
631-653:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDon’t wrap a nil error when no nodes are returned.
At Line 652,
%wwrapserr, buterris nil on this path. Return a direct error message instead.Suggested fix
- if len(nodes.Items) < 1 { - return "", fmt.Errorf("no node objects found: %w", err) - } + if len(nodes.Items) < 1 { + return "", errors.New("no node objects found") + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/cluster/core/create.go` around lines 631 - 653, In GetAPIServerAddressByNode, the code returns fmt.Errorf("no node objects found: %w", err) while err is nil; change this to return a direct error message (e.g. return "", fmt.Errorf("no node objects found")) so you don't wrap a nil error. Locate the nodes.Items length check in GetAPIServerAddressByNode and replace the faulty fmt.Errorf call with a plain error string or include relevant context without using %w.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@cmd/cluster/core/create.go`:
- Around line 631-653: In GetAPIServerAddressByNode, the code returns
fmt.Errorf("no node objects found: %w", err) while err is nil; change this to
return a direct error message (e.g. return "", fmt.Errorf("no node objects
found")) so you don't wrap a nil error. Locate the nodes.Items length check in
GetAPIServerAddressByNode and replace the faulty fmt.Errorf call with a plain
error string or include relevant context without using %w.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 0118075a-4f36-4f58-a6bd-469e00e90bd3
📒 Files selected for processing (12)
cmd/cluster/aws/create.gocmd/cluster/aws/create_test.gocmd/cluster/aws/destroy.gocmd/cluster/aws/destroy_test.gocmd/cluster/cluster.gocmd/cluster/core/create.gocmd/util/client.gocmd/util/client_test.goproduct-cli/cmd/cluster/aws/destroy.goproduct-cli/cmd/cluster/cluster.gosupport/util/clientset_test.gosupport/util/util.go
✅ Files skipped from review due to trivial changes (3)
- product-cli/cmd/cluster/cluster.go
- support/util/clientset_test.go
- cmd/util/client.go
🚧 Files skipped from review as they are similar to previous changes (5)
- cmd/cluster/aws/create_test.go
- cmd/cluster/aws/destroy_test.go
- cmd/cluster/cluster.go
- cmd/util/client_test.go
- cmd/cluster/aws/destroy.go
|
Re: #8402 (review) Good catch — fixed. At line 652, AI-assisted response via Claude Code |
bf93e3b to
842c49f
Compare
842c49f to
f45715d
Compare
|
Stale PRs are closed after 21d of inactivity. If this PR is still relevant, comment to refresh it or remove the stale label. If this PR is safe to close now please do so with /lifecycle stale |
openshift-ci
Bot
added
the
lifecycle/stale
|
/remove-lifecycle stale |
openshift-ci
Bot
removed
the
lifecycle/stale
|
rebase this PR |
|
Re: #8402 (comment) Done. Rebased all 4 commits onto the latest AI-assisted response via Claude Code |
|
/rebase |
Improve patch coverage for the --kubeconfig changes by adding tests for ValidateCredentialInfo with credential secrets, BindOptions kubeconfig flag parsing, GetCluster with invalid kubeconfig, and wrapper functions GetConfig, GetClient, and GetKubeClientSet. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
9ad4e33 to
9ab3ce7
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bryan-cox, hypershift-jira-solve-ci[bot] 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 |
openshift-ci
Bot
added
the
approved
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
/retest |
|
Why are gcp and azure clusters not included in this change? |
|
/address-review-comments |
|
^testing this out some more |
|
🤖 Addressing review comments: workflow run |
|
GCP and Azure are included in this change. The The platform-specific file changes in this PR were only needed where platform code created its own Kubernetes clients directly — AWS had Azure and GCP don't have any such platform-specific client creation — they delegate entirely to AI-assisted response via Claude Code |
|
/lgtm |
|
Scheduling tests matching the |
|
/verified by UT |
openshift-ci-robot
added
the
verified
|
@bryan-cox: This PR has been marked as verified by DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
All 12 Now I have all the evidence I need for a comprehensive report. All 6 jobs failed with the identical root cause: Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryAll 6 CI jobs failed with an identical infrastructure error unrelated to the PR's code changes. The Go compilation stage succeeded — all binaries ( Root CauseCategory: External Infrastructure Failure (Microsoft RPM Repository GPG Signing) The RUN rpm --import https://packages.microsoft.com/keys/microsoft.asc && \
dnf install -y https://packages.microsoft.com/config/rhel/9/packages-microsoft-prod.rpm && \
mv /etc/yum.repos.d/microsoft-prod.repo /etc/yum.repos.art/ci/ && \
dnf install -y azure-cli && \
dnf clean allThe failure chain:
This is NOT caused by the PR changes. The PR adds a Additionally, DNS instability was observed ( Recommendations
Evidence
|
cblecker
left a comment
cblecker
left a comment
There was a problem hiding this comment.
Minor note: GetImpersonatedClient (cmd/util/client.go) still calls GetConfig() directly while all other client-creation functions now delegate to their WithKubeconfig variants. This is fine since the dump command is out of scope, but worth noting if --kubeconfig is extended to other commands later.
| g.Expect(receivedOpts.AzurePlatform.Cloud).To(Equal("AzurePublicCloud")) | ||
| }) | ||
|
|
||
| t.Run("When kubeconfig is set it should use it for the client", func(t *testing.T) { |
There was a problem hiding this comment.
This test is named "When kubeconfig is set it should use it for the client" but sets Kubeconfig: "" and relies on FAKE_CLIENT=true, which short-circuits GetClientWithKubeconfig before the kubeconfig path is ever consulted. It ends up being functionally identical to the test above it (lines 14-44).
Consider either renaming it to reflect what it actually tests, or rewriting it to exercise the kubeconfig path — e.g., using writeTestKubeconfig to provide a real path and dropping FAKE_CLIENT.
There was a problem hiding this comment.
Thanks! I'll follow up on this one.
| // ValidateCredentialInfo validates if the credentials secret name is empty, the aws-creds or sts-creds mutually exclusive and are not empty; validates if | ||
| // the credentials secret is not empty, that it can be retrieved. | ||
| func ValidateCredentialInfo(opts awsutil.AWSCredentialsOptions, credentialSecretName, namespace string) error { | ||
| func ValidateCredentialInfo(opts awsutil.AWSCredentialsOptions, credentialSecretName, namespace, kubeconfigPath string) error { |
There was a problem hiding this comment.
Nit for a follow-up: ValidateCredentialInfo now has 3 consecutive string parameters (credentialSecretName, namespace, kubeconfigPath), and ValidateCreateCredentialInfo has 4. Transposing any two compiles cleanly but produces wrong behavior. A small params struct would make call sites self-documenting. Not blocking this PR — callers are all correct today.
There was a problem hiding this comment.
Thanks! I'll follow up on this one.
|
/retest |
1 similar comment
|
/retest |
|
I now have all the evidence needed. Let me produce the final report. Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryThe Root CauseThe root cause is a test sequencing/timing issue in the etcd chaos test suite, not a product bug or anything related to this PR. Detailed sequence of events:
Why etcd reported "unhealthy cluster": In etcd, a PR #8402 is not involved: The PR modifies only CLI flag handling for Recommendations
Evidence
|
|
@hypershift-jira-solve-ci: all tests passed! 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. |
4 participants
What this PR does / why we need it:
Adds an explicit
--kubeconfigflag to thecreate clusteranddestroy clustercommands in both thehypershiftandhcpCLIs.Previously, users managing multiple HyperShift management clusters had to modify their default kubeconfig or set the
KUBECONFIGenvironment variable before running CLI commands. The new--kubeconfigflag provides a more direct and scriptable approach to target a specific management cluster. When the flag is not provided, the CLI falls back to the default kubeconfig resolution (KUBECONFIGenv var, then~/.kube/config).Key changes:
GetConfigWithKubeconfig,GetClientWithKubeconfig, andGetKubeClientSetWithKubeconfigfunctions that accept an explicit kubeconfig path, falling back to default resolution when empty--kubeconfigas a persistent flag on bothcreate clusteranddestroy clusterparent commands (shared acrosshypershiftandhcpCLIs viabindCoreOptionsand direct flag registration)GetSecretcalls that previously bypassed the flagGetConfig,GetClient, andGetKubeClientSetto delegate to theirWithKubeconfigvariants to eliminate duplicationWhich issue(s) this PR fixes:
Fixes https://redhat.atlassian.net/browse/CNTRLPLANE-596
Special notes for your reviewer:
--kubeconfigflag is only added tocreate clusteranddestroy clustercommands, not to all subcommands. This is intentional to limit scope.GetAPIServerAddressByNodewas changed from variadic...stringto a plainstringparameter for the kubeconfig path, since it always receives exactly one value.Checklist:
Always review AI generated responses prior to use.
Generated with Claude Code via
/jira:solve [CNTRLPLANE-596](https://redhat.atlassian.net/browse/CNTRLPLANE-596)Summary by CodeRabbit
New Features
Tests