SREP-3937: Add --hive-ocm-url flag to cluster resize infra for multi-env OCM support#868
Conversation
openshift-ci
bot
added
the
do-not-merge/hold
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds a new optional Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip Migrating from UI to YAML configuration.Use the |
|
@nephomaniac: An error was encountered searching for bug SREP-3937 on the Jira server at https://issues.redhat.com. No known errors were detected, please see the full error message for details. Full error message.
No response returned: Get "https://issues.redhat.com/rest/api/2/issue/SREP-3937": GET https://issues.redhat.com/rest/api/2/issue/SREP-3937 giving up after 5 attempt(s)
Please contact an administrator to resolve this issue, then request a bug refresh with 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: nephomaniac The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
cmd/cluster/resize/infra_node.go (2)
118-124: Use the resolved OCM URL for connection creation.Line 120 resolves the input, but the resolved value is discarded and Line 149 still uses the raw flag string. Persisting the resolved value keeps validation and connection behavior strictly aligned.
Suggested change
// Validate --hive-ocm-url if provided if r.hiveOcmUrl != "" { - _, err := utils.ValidateAndResolveOcmUrl(r.hiveOcmUrl) + resolvedHiveOcmURL, err := utils.ValidateAndResolveOcmUrl(r.hiveOcmUrl) if err != nil { return fmt.Errorf("invalid --hive-ocm-url: %w", err) } + r.hiveOcmUrl = resolvedHiveOcmURL }Also applies to: 149-151
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/cluster/resize/infra_node.go` around lines 118 - 124, The validation discards the resolved OCM URL returned by utils.ValidateAndResolveOcmUrl and later code still uses the raw r.hiveOcmUrl when creating the connection; change the validation call in the infra node flow to capture the resolved value (e.g., resolvedHiveOcmUrl, or overwrite r.hiveOcmUrl) and then use that resolved value when constructing the connection (the code that currently references r.hiveOcmUrl for connection creation). Ensure you call utils.ValidateAndResolveOcmUrl only once, persist its returned URL, and pass that persisted resolved URL into the connection creation code paths that currently read r.hiveOcmUrl.
141-221: Reduce duplicated client/bootstrap logic across both paths.The two branches repeat most setup steps (cluster lookup,
r.clusterIdassignment, client creation, admin client creation). Extracting shared setup into helpers will lower drift risk and make future changes safer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/cluster/resize/infra_node.go` around lines 141 - 221, The two branches for r.hiveOcmUrl share cluster lookup, r.clusterId assignment and k8s client creation; extract that shared setup into helper(s). Create a helper like prepareClusterAndClients(targetConn utils.OCMConnection, hiveConn utils.OCMConnection, hiveOcmUrl string) (or two helpers: one to resolve cluster via utils.GetClusterAnyStatus and set r.cluster/r.clusterId, and one to create k8s clients) that calls utils.GetClusterAnyStatus, assigns r.cluster and r.clusterId, and returns c, hc, hac (or returns error). In the hive-path call utils.CreateConnection and utils.CreateConnectionWithUrl and pass both conns and hiveOcmUrl into the helper so it can choose k8s.NewWithConn / k8s.NewAsBackplaneClusterAdminWithConn (preserving error wrapping with the hiveOcmUrl); in the non-hive path call utils.CreateConnection and pass nil hiveConn so the helper uses k8s.New / k8s.NewAsBackplaneClusterAdmin. Ensure all original error messages that referenced r.hiveOcmUrl remain when hiveOcmUrl is non-empty.
🤖 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/cluster/resize/infra_node_test.go`:
- Around line 507-572: Test Hive OCM URL validation is calling
utils.ValidateAndResolveOcmUrl directly and expects empty input to error, but
Infra.New() treats an empty --hive-ocm-url as optional; update
TestHiveOcmUrlValidation to exercise the same path as the command by invoking
Infra.New() (or a thin wrapper that mirrors its preflight behavior) instead of
calling utils.ValidateAndResolveOcmUrl directly, adjust the test cases so empty
hiveOcmUrl expects no error, and remove the redundant conditional that calls
ValidateAndResolveOcmUrl twice; reference TestHiveOcmUrlValidation, Infra.New,
and utils.ValidateAndResolveOcmUrl when making the change.
---
Nitpick comments:
In `@cmd/cluster/resize/infra_node.go`:
- Around line 118-124: The validation discards the resolved OCM URL returned by
utils.ValidateAndResolveOcmUrl and later code still uses the raw r.hiveOcmUrl
when creating the connection; change the validation call in the infra node flow
to capture the resolved value (e.g., resolvedHiveOcmUrl, or overwrite
r.hiveOcmUrl) and then use that resolved value when constructing the connection
(the code that currently references r.hiveOcmUrl for connection creation).
Ensure you call utils.ValidateAndResolveOcmUrl only once, persist its returned
URL, and pass that persisted resolved URL into the connection creation code
paths that currently read r.hiveOcmUrl.
- Around line 141-221: The two branches for r.hiveOcmUrl share cluster lookup,
r.clusterId assignment and k8s client creation; extract that shared setup into
helper(s). Create a helper like prepareClusterAndClients(targetConn
utils.OCMConnection, hiveConn utils.OCMConnection, hiveOcmUrl string) (or two
helpers: one to resolve cluster via utils.GetClusterAnyStatus and set
r.cluster/r.clusterId, and one to create k8s clients) that calls
utils.GetClusterAnyStatus, assigns r.cluster and r.clusterId, and returns c, hc,
hac (or returns error). In the hive-path call utils.CreateConnection and
utils.CreateConnectionWithUrl and pass both conns and hiveOcmUrl into the helper
so it can choose k8s.NewWithConn / k8s.NewAsBackplaneClusterAdminWithConn
(preserving error wrapping with the hiveOcmUrl); in the non-hive path call
utils.CreateConnection and pass nil hiveConn so the helper uses k8s.New /
k8s.NewAsBackplaneClusterAdmin. Ensure all original error messages that
referenced r.hiveOcmUrl remain when hiveOcmUrl is non-empty.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b56f40e0-9f36-488b-af3b-8e0dc3a6f6e6
📒 Files selected for processing (4)
cmd/cluster/resize/infra_node.gocmd/cluster/resize/infra_node_test.godocs/README.mddocs/osdctl_cluster_resize_infra.md
nephomaniac
force-pushed
the
SREP-3937-resize-infra-multi-env-ocm
branch
from
5485f25 to
24e5982
Compare
|
/retest |
1 similar comment
|
/retest |
|
@nephomaniac: 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. |
|
/hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
2 participants
Summary
Adds
--hive-ocm-urlflag toosdctl cluster resize infracommand to enable multi-environment OCM support for staging/integration testing.Changes
--hive-ocm-urloptional flag to cluster resize infra commandNew()method to fail fast on invalid OCM URLsinfra_node_test.goto verify early validation behaviorImplementation Details
When
--hive-ocm-urlis specified:utils.CreateConnectionWithUrl()utils.GetHiveClusterWithConn()to query hive from different OCM environmentk8s.NewWithConn()utils.ValidateAndResolveOcmUrl()Testing
Related
Note
This PR is part of a series adding multi-environment OCM support to osdctl commands. Will place on hold until the common implementation pattern can be reviewed across all related PRs.
Co-Authored-By: Claude Sonnet 4.5 noreply@anthropic.com