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 |
WalkthroughAdds an optional --hive-ocm-url flag and multi-environment initialization path to the cluster resync command. When set, Resync initializes separate OCM connections (target and hive), resolves the hive cluster, creates a hive k8s client, and routes deletion readiness through initWithMultiEnv; previous behavior remains 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
| 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 Resync.New() method | ||
| var err error | ||
| if tt.hiveOcmUrl != "" { | ||
| _, err = utils.ValidateAndResolveOcmUrl(tt.hiveOcmUrl) | ||
| } else { | ||
| _, err = utils.ValidateAndResolveOcmUrl(tt.hiveOcmUrl) | ||
| } |
There was a problem hiding this comment.
Test logic for empty hiveOcmUrl does not match command behavior.
In cmd/cluster/resync.go, Line 76 validates only when the flag is non-empty. This test currently expects an error for empty input and Lines 56-60 call validation unconditionally (identical if/else), so it doesn’t actually simulate Resync.New() and can mask regressions in backward-compatible behavior.
Suggested test correction
{
- name: "Empty hive-ocm-url",
- hiveOcmUrl: "",
- expectErr: true,
- errContains: "empty OCM URL",
+ name: "Empty hive-ocm-url (flag omitted)",
+ hiveOcmUrl: "",
+ expectErr: false,
},
}
@@
- var err error
- if tt.hiveOcmUrl != "" {
- _, err = utils.ValidateAndResolveOcmUrl(tt.hiveOcmUrl)
- } else {
- _, err = utils.ValidateAndResolveOcmUrl(tt.hiveOcmUrl)
- }
+ var err error
+ // Mirrors Resync.New(): validate only when flag is provided.
+ if tt.hiveOcmUrl != "" {
+ _, err = utils.ValidateAndResolveOcmUrl(tt.hiveOcmUrl)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 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 Resync.New() method | |
| var err error | |
| if tt.hiveOcmUrl != "" { | |
| _, err = utils.ValidateAndResolveOcmUrl(tt.hiveOcmUrl) | |
| } else { | |
| _, err = utils.ValidateAndResolveOcmUrl(tt.hiveOcmUrl) | |
| } | |
| name: "Empty hive-ocm-url (flag omitted)", | |
| hiveOcmUrl: "", | |
| expectErr: false, | |
| }, | |
| } | |
| for _, tt := range tests { | |
| t.Run(tt.name, func(t *testing.T) { | |
| // This simulates the validation that occurs in the Resync.New() method | |
| var err error | |
| // Mirrors Resync.New(): validate only when flag is provided. | |
| if tt.hiveOcmUrl != "" { | |
| _, 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/resync_test.go` around lines 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().
| ``` | ||
| -C, --cluster-id string OCM internal/external cluster id or cluster name to delete the clustersync for. | ||
| -h, --help help for resync | ||
| -C, --cluster-id string OCM internal/external cluster id or cluster name to delete the clustersync for. | ||
| -h, --help help for resync | ||
| --hive-ocm-url string (optional) OCM environment URL for hive operations. Aliases: 'production', 'staging', 'integration'. If not specified, uses the same OCM environment as the target cluster. | ||
| ``` |
There was a problem hiding this comment.
Add a language identifier to the fenced Options block.
markdownlint MD040 is triggered at Line 33 because the fence has no language token.
Suggested doc fix
-```
+```text
-C, --cluster-id string OCM internal/external cluster id or cluster name to delete the clustersync for.
-h, --help help for resync
--hive-ocm-url string (optional) OCM environment URL for hive operations. Aliases: 'production', 'staging', 'integration'. If not specified, uses the same OCM environment as the target cluster.</details>
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.21.0)</summary>
[warning] 33-33: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
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 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.
</details>
<!-- fingerprinting:phantom:poseidon:hawk -->
<!-- This is an auto-generated comment by CodeRabbit -->
|
@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. |
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