SREP-3937: Add --hive-ocm-url flag to cluster resize infra for multi-env OCM support#868
Conversation
|
Placing on hold until all the similar commands/PRs adding multiple OCM env support can have the common impl pattern reviewed. Will unhold after the other PRs, and general pattern get a chance for review. /hold |
openshift-ci
bot
added
the
do-not-merge/hold
WalkthroughAdded 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 |
|
@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 |
|
@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. |
![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
| // TestHiveOcmUrlValidation tests the early validation of --hive-ocm-url flag in the infra resize command | ||
| func TestHiveOcmUrlValidation(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| hiveOcmUrl string | ||
| expectErr bool | ||
| errContains string | ||
| }{ | ||
| { | ||
| name: "Valid hive-ocm-url (production)", | ||
| hiveOcmUrl: "production", | ||
| expectErr: false, | ||
| }, | ||
| { | ||
| name: "Valid hive-ocm-url (staging)", | ||
| hiveOcmUrl: "staging", | ||
| expectErr: false, | ||
| }, | ||
| { | ||
| name: "Valid hive-ocm-url (integration)", | ||
| hiveOcmUrl: "integration", | ||
| expectErr: false, | ||
| }, | ||
| { | ||
| name: "Valid hive-ocm-url (full URL)", | ||
| hiveOcmUrl: "https://api.openshift.com", | ||
| expectErr: false, | ||
| }, | ||
| { | ||
| name: "Invalid hive-ocm-url", | ||
| hiveOcmUrl: "invalid-environment", | ||
| expectErr: true, | ||
| errContains: "invalid OCM_URL", | ||
| }, | ||
| { | ||
| name: "Empty hive-ocm-url", | ||
| hiveOcmUrl: "", | ||
| expectErr: true, | ||
| errContains: "empty OCM URL", | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| // This simulates the validation that occurs in the Infra.New() method | ||
| var err error | ||
| if tt.hiveOcmUrl != "" { | ||
| _, err = utils.ValidateAndResolveOcmUrl(tt.hiveOcmUrl) | ||
| } else { | ||
| _, err = utils.ValidateAndResolveOcmUrl(tt.hiveOcmUrl) | ||
| } | ||
|
|
||
| if tt.expectErr { | ||
| if err == nil { | ||
| t.Errorf("Expected error containing '%s', but got nil", tt.errContains) | ||
| } else if !strings.Contains(err.Error(), tt.errContains) { | ||
| t.Errorf("Expected error containing '%s', but got: %v", tt.errContains, err) | ||
| } | ||
| } else { | ||
| if err != nil { | ||
| t.Errorf("Expected no error, but got: %v", err) | ||
| } | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Test assertions do not match Infra.New() behavior.
This test claims to cover early validation in Infra.New(), but it calls utils.ValidateAndResolveOcmUrl directly. It also expects empty input to error, while Infra.New() skips validation when the flag is empty (optional flag behavior). That makes the test pass while not protecting the actual command path.
Suggested correction (align test semantics with command behavior)
{
name: "Empty hive-ocm-url",
hiveOcmUrl: "",
- expectErr: true,
- errContains: "empty OCM URL",
+ expectErr: false,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
- // This simulates the validation that occurs in the Infra.New() method
+ // Simulate Infra.New() behavior: validate only when value is provided
var err error
if tt.hiveOcmUrl != "" {
_, err = utils.ValidateAndResolveOcmUrl(tt.hiveOcmUrl)
- } else {
- _, err = utils.ValidateAndResolveOcmUrl(tt.hiveOcmUrl)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/cluster/resize/infra_node_test.go` around lines 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.
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