SREP-3896: Add --hive-ocm-url flag to cluster resync for multi-env OCM support#866
SREP-3896: Add --hive-ocm-url flag to cluster resync for multi-env OCM support#866nephomaniac wants to merge 1 commit intoopenshift:masterfrom
Conversation
|
@nephomaniac: An error was encountered searching for bug SREP-3896 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-3896": GET https://issues.redhat.com/rest/api/2/issue/SREP-3896 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. |
|
Placing on hold until all the similar commands/PRs adding multiple OCM env support can have the common implementation pattern reviewed. Will unhold after the other PRs and general pattern get a chance for review. /hold |
|
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:
WalkthroughAdds an optional --hive-ocm-url flag and multi-environment initialization to the cluster resync command. When provided, Resync validates and resolves the hive OCM URL, creates separate OCM clients for target and hive, resolves the hive cluster mapping, and builds a hive Kubernetes client via a new initWithMultiEnv path; unchanged behavior when unset. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
|
[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 |
openshift-ci
bot
added
the
do-not-merge/hold
![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 (1)
cmd/cluster/resync.go (1)
83-87: Consider collapsing the init branching into one helper.These new branches now mirror much of the legacy init flow with only the connection source differing. Keeping both paths separate makes future fixes easy to land in one branch but not the other.
As per coding guidelines, Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Also applies to: 116-156
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/cluster/resync.go` around lines 83 - 87, The branching between the multi-environment path (r.initWithMultiEnv) and the legacy init is duplicating most of the init flow; refactor by extracting a single shared initializer that accepts a connection/source provider and runs the common setup logic. Implement a small helper (e.g., buildConnectionSource or getInitSource) that encapsulates the differing parts (the hive OCM URL path vs. legacy connection creation) and then call a single common init function (the current shared setup logic from r.initWithMultiEnv / r.init) with that provider so all environment-specific differences are isolated to the provider and the rest of the initialization is unified.
🤖 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/resync.go`:
- Around line 49-50: The example/help text for the resync command is misleading
about --hive-ocm-url; update the wording in the osdctl cluster resync
help/example so it explicitly states that the --hive-ocm-url flag only changes
which OCM endpoint is used to contact Hive and does NOT change how the target
cluster is resolved (New() still resolves the target cluster from the
current/default OCM connection). Mention this prerequisite and clarify that
cluster selection remains tied to the existing/default OCM client while only the
Hive side uses the provided URL.
---
Nitpick comments:
In `@cmd/cluster/resync.go`:
- Around line 83-87: The branching between the multi-environment path
(r.initWithMultiEnv) and the legacy init is duplicating most of the init flow;
refactor by extracting a single shared initializer that accepts a
connection/source provider and runs the common setup logic. Implement a small
helper (e.g., buildConnectionSource or getInitSource) that encapsulates the
differing parts (the hive OCM URL path vs. legacy connection creation) and then
call a single common init function (the current shared setup logic from
r.initWithMultiEnv / r.init) with that provider so all environment-specific
differences are isolated to the provider and the rest of the initialization is
unified.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f598db0b-edba-403a-a2b0-0a8f68474fb7
📒 Files selected for processing (3)
cmd/cluster/resync.godocs/README.mddocs/osdctl_cluster_resync.md
nephomaniac
force-pushed
the
SREP-3896-resync-multi-env-ocm
branch
from
314765f to
b1a86c4
Compare
|
@nephomaniac: An error was encountered searching for bug SREP-3896 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-3896": GET https://issues.redhat.com/rest/api/2/issue/SREP-3896 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. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/resync_test.go`:
- Around line 45-60: The test in resync_test.go incorrectly calls
utils.ValidateAndResolveOcmUrl unconditionally (the if/else blocks are
identical) and expects an error for an empty hiveOcmUrl, but Resync.New() only
validates when the flag is non-empty; update the test to mimic Resync.New():
call utils.ValidateAndResolveOcmUrl only when tt.hiveOcmUrl != "" (otherwise
skip validation) and adjust the test expectations for the empty hiveOcmUrl case
(set expectErr=false / remove errContains) so the test verifies the same
behavior as Resync.New().
In `@docs/osdctl_cluster_resync.md`:
- Around line 33-37: The fenced code block showing the CLI Options lacks a
language identifier and triggers markdownlint MD040; update the Options fenced
block in docs/osdctl_cluster_resync.md by adding a language token (e.g., use
```text) immediately after the opening backticks for the block containing the
three option lines so the block becomes a labeled code fence and resolves the
lint warning.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3a8be291-a525-4990-883d-8ff4216c765b
📒 Files selected for processing (4)
cmd/cluster/resync.gocmd/cluster/resync_test.godocs/README.mddocs/osdctl_cluster_resync.md
nephomaniac
force-pushed
the
SREP-3896-resync-multi-env-ocm
branch
2 times, most recently
from
55e6c52 to
7df8eee
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/cluster/resync.go`:
- Around line 75-81: The validation call to ValidateAndResolveOcmUrl returns a
resolved URL but its value is discarded; capture that resolved value and use it
for the Hive connection instead of the original r.hiveOcmUrl. Specifically, in
the pre-check where ValidateAndResolveOcmUrl is called, assign the returned
string (e.g., resolvedOcmUrl) and either set r.hiveOcmUrl = resolvedOcmUrl or
pass resolvedOcmUrl into the later connection setup that currently references
r.hiveOcmUrl (ensure functions/config that use the Hive OCM URL use the resolved
variable).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e79b4492-f64c-49b2-8dfd-2ecfa47b9931
📒 Files selected for processing (4)
cmd/cluster/resync.gocmd/cluster/resync_test.godocs/README.mddocs/osdctl_cluster_resync.md
nephomaniac
force-pushed
the
SREP-3896-resync-multi-env-ocm
branch
from
7df8eee to
6225a27
Compare
|
@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. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
docs/osdctl_cluster_resync.md (1)
33-37:⚠️ Potential issue | 🟡 MinorAdd a language identifier to the Options fenced block.
The fence is still unlabeled and triggers markdownlint MD040; use a labeled fence like
```text.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/osdctl_cluster_resync.md` around lines 33 - 37, The fenced Options block shown (the triple-backtick block containing the option lines "-C, --cluster-id ...", "-h, --help ...", and "--hive-ocm-url ...") lacks a language identifier and triggers markdownlint MD040; update the opening fence from ``` to ```text (i.e., label the code fence with "text") so the block becomes ```text and the linter stops flagging it.
🧹 Nitpick comments (1)
cmd/cluster/resync.go (1)
90-157: Consider extracting shared init steps to reduce path drift.The legacy path and
initWithMultiEnvrepeat core initialization steps (cluster resolution, Hive lookup, client assignment/logging). A shared helper would improve maintainability and reduce future divergence bugs.As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/cluster/resync.go` around lines 90 - 157, The two init paths duplicate cluster resolution, hive lookup and hive k8s client creation; extract these into a shared helper (e.g., resolveClusterAndHive or initHiveClient) that accepts the OCM connection(s) and clusterId and returns the resolved cluster (ID/Name), hive name/ID and the k8s client; then have the legacy path (which uses utils.CreateConnection, utils.GetClusterAnyStatus, utils.GetHiveCluster, k8s.New) and initWithMultiEnv (which uses CreateConnection/CreateConnectionWithUrl, utils.GetClusterAnyStatus, utils.GetHiveClusterWithConn, k8s.NewWithConn) call that helper to perform the common steps and only keep connection-creation/logging differences in each path to eliminate duplication and reduce path drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@docs/osdctl_cluster_resync.md`:
- Around line 33-37: The fenced Options block shown (the triple-backtick block
containing the option lines "-C, --cluster-id ...", "-h, --help ...", and
"--hive-ocm-url ...") lacks a language identifier and triggers markdownlint
MD040; update the opening fence from ``` to ```text (i.e., label the code fence
with "text") so the block becomes ```text and the linter stops flagging it.
---
Nitpick comments:
In `@cmd/cluster/resync.go`:
- Around line 90-157: The two init paths duplicate cluster resolution, hive
lookup and hive k8s client creation; extract these into a shared helper (e.g.,
resolveClusterAndHive or initHiveClient) that accepts the OCM connection(s) and
clusterId and returns the resolved cluster (ID/Name), hive name/ID and the k8s
client; then have the legacy path (which uses utils.CreateConnection,
utils.GetClusterAnyStatus, utils.GetHiveCluster, k8s.New) and initWithMultiEnv
(which uses CreateConnection/CreateConnectionWithUrl, utils.GetClusterAnyStatus,
utils.GetHiveClusterWithConn, k8s.NewWithConn) call that helper to perform the
common steps and only keep connection-creation/logging differences in each path
to eliminate duplication and reduce path drift.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c62501ce-d7c0-498b-b323-63d6414a9df2
📒 Files selected for processing (4)
cmd/cluster/resync.gocmd/cluster/resync_test.godocs/README.mddocs/osdctl_cluster_resync.md
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/cluster/resync_test.go
- docs/README.md
2 participants
Summary
Adds
--hive-ocm-urlflag toosdctl cluster resynccommand to enable multi-environment OCM support for staging/integration testing.Changes
--hive-ocm-urloptional flag to cluster resync commandNew()method to fail fast on invalid OCM URLsresync_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