SREP-3938: Add --hive-ocm-url flag to network verify-egress for multi-env OCM support#867
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
|
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 multi-environment Hive/OCM support to the verify-egress command by introducing a 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 |
|
@nephomaniac: An error was encountered searching for bug SREP-3938 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-3938": GET https://issues.redhat.com/rest/api/2/issue/SREP-3938 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.
🧹 Nitpick comments (3)
cmd/network/verification.go (2)
170-173: Clarify when this example actually exercises--hive-ocm-url.This reads like a general multi-environment switch, but in this command's current implementation the flag is only consulted on the Hive-backed CA-bundle lookup path. Calling out the classic/proxy CA-bundle case here will set the right expectation and avoid confusing no-op runs.
Possible wording
- # Run against a staging cluster using production Hive (multi-environment OCM) + # For a classic cluster that needs automatic proxy CA-bundle retrieval, + # target staging OCM while querying Hive from productionAs 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/network/verification.go` around lines 170 - 173, Update the example text for the osdctl network verify-egress invocation to explicitly state that the --hive-ocm-url flag is only consulted when the command follows the Hive-backed CA-bundle lookup path (i.e., when the code path that queries Hive for CA bundles is used) and is effectively a no-op for the classic/proxy CA-bundle lookup path; mention the env var OCM_URL and the command osdctl network verify-egress and the flag --hive-ocm-url so readers know when the flag matters.
455-497: Extract the Hive client bootstrap into a helper.Both branches resolve the Hive cluster and then build a Kubernetes client, with only the connection source changing. Pulling that into a helper would remove duplication here and make the new multi-env path easier to unit-test.
Possible shape
func (e *EgressVerification) getCaBundleFromHive(ctx context.Context) (string, error) { scheme := runtime.NewScheme() if err := corev1.AddToScheme(scheme); err != nil { return "", err } if err := hivev1.AddToScheme(scheme); err != nil { return "", err } - var hive *cmv1.Cluster - var hc client.Client - var err error - - if e.hiveOcmUrl != "" { - ... - } else { - ... - } + hive, hc, err := e.getHiveClient(ctx, scheme) + if err != nil { + return "", err + } e.log.Debug(ctx, "searching for proxy SyncSet") ... }func (e *EgressVerification) getHiveClient(ctx context.Context, scheme *runtime.Scheme) (*cmv1.Cluster, client.Client, error) { // keep the single-env and multi-env connection/client setup here }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/network/verification.go` around lines 455 - 497, Extract the duplicated logic that resolves a Hive cluster and builds a Kubernetes client into a helper on EgressVerification (e.g. func (e *EgressVerification) getHiveClient(ctx context.Context, scheme *runtime.Scheme) (*cmv1.Cluster, client.Client, error)); inside it handle both paths: when e.hiveOcmUrl != "" call utils.CreateConnection(), utils.CreateConnectionWithUrl(e.hiveOcmUrl) and utils.GetHiveClusterWithConn(e.cluster.ID(), targetOCM, hiveOCM) and create the k8s client with k8s.NewWithConn(hive.ID(), client.Options{Scheme: scheme}, hiveOCM) (ensuring you defer Close on connections inside caller context or return connections if needed), otherwise call utils.GetHiveCluster(e.cluster.ID()) and k8s.New(hive.ID(), client.Options{Scheme: scheme}); then replace the duplicated block in verification.go with a single call to e.getHiveClient(ctx, scheme) and propagate returned errors.cmd/network/verification_test.go (1)
177-182: Remove redundant conditional - both branches are identical.The if/else block executes the same code in both branches, making the conditional unnecessary.
♻️ Simplify by removing the conditional
- // This simulates the validation that occurs in the validateInput() method - var err error - if tt.hiveOcmUrl != "" { - _, err = utils.ValidateAndResolveOcmUrl(tt.hiveOcmUrl) - } else { - _, err = utils.ValidateAndResolveOcmUrl(tt.hiveOcmUrl) - } + // This simulates the validation that occurs in the validateInput() method + _, err := utils.ValidateAndResolveOcmUrl(tt.hiveOcmUrl)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/network/verification_test.go` around lines 177 - 182, The if/else block around calling utils.ValidateAndResolveOcmUrl is redundant because both branches call the same function with tt.hiveOcmUrl; remove the conditional and call utils.ValidateAndResolveOcmUrl(tt.hiveOcmUrl) once, assigning to the existing err variable (and discarding the first return value if unused) to simplify the test code that references ValidateAndResolveOcmUrl and tt.hiveOcmUrl.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cmd/network/verification_test.go`:
- Around line 177-182: The if/else block around calling
utils.ValidateAndResolveOcmUrl is redundant because both branches call the same
function with tt.hiveOcmUrl; remove the conditional and call
utils.ValidateAndResolveOcmUrl(tt.hiveOcmUrl) once, assigning to the existing
err variable (and discarding the first return value if unused) to simplify the
test code that references ValidateAndResolveOcmUrl and tt.hiveOcmUrl.
In `@cmd/network/verification.go`:
- Around line 170-173: Update the example text for the osdctl network
verify-egress invocation to explicitly state that the --hive-ocm-url flag is
only consulted when the command follows the Hive-backed CA-bundle lookup path
(i.e., when the code path that queries Hive for CA bundles is used) and is
effectively a no-op for the classic/proxy CA-bundle lookup path; mention the env
var OCM_URL and the command osdctl network verify-egress and the flag
--hive-ocm-url so readers know when the flag matters.
- Around line 455-497: Extract the duplicated logic that resolves a Hive cluster
and builds a Kubernetes client into a helper on EgressVerification (e.g. func (e
*EgressVerification) getHiveClient(ctx context.Context, scheme *runtime.Scheme)
(*cmv1.Cluster, client.Client, error)); inside it handle both paths: when
e.hiveOcmUrl != "" call utils.CreateConnection(),
utils.CreateConnectionWithUrl(e.hiveOcmUrl) and
utils.GetHiveClusterWithConn(e.cluster.ID(), targetOCM, hiveOCM) and create the
k8s client with k8s.NewWithConn(hive.ID(), client.Options{Scheme: scheme},
hiveOCM) (ensuring you defer Close on connections inside caller context or
return connections if needed), otherwise call
utils.GetHiveCluster(e.cluster.ID()) and k8s.New(hive.ID(),
client.Options{Scheme: scheme}); then replace the duplicated block in
verification.go with a single call to e.getHiveClient(ctx, scheme) and propagate
returned errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 06df6dde-0fa9-4949-bb60-973ebb0b528a
📒 Files selected for processing (4)
cmd/network/verification.gocmd/network/verification_test.godocs/README.mddocs/osdctl_network_verify-egress.md
nephomaniac
force-pushed
the
SREP-3938-verify-egress-multi-env-ocm
branch
from
93b5020 to
2c27d92
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/network/verification.go (1)
679-685: Consider storing the resolved URL to avoid redundant resolution.The validation correctly fails early on invalid input, but the resolved URL is discarded. Since
CreateConnectionWithUrlcallsValidateAndResolveOcmUrlinternally anyway, you're resolving the same URL twice. Storing the resolved value avoids this redundancy:♻️ Suggested fix
// Validate --hive-ocm-url if provided if e.hiveOcmUrl != "" { - _, err := utils.ValidateAndResolveOcmUrl(e.hiveOcmUrl) + resolvedUrl, err := utils.ValidateAndResolveOcmUrl(e.hiveOcmUrl) if err != nil { return fmt.Errorf("invalid --hive-ocm-url: %w", err) } + e.hiveOcmUrl = resolvedUrl }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/network/verification.go` around lines 679 - 685, The current validation calls utils.ValidateAndResolveOcmUrl(e.hiveOcmUrl) but discards the resolved URL, causing a second resolution later in CreateConnectionWithUrl; capture and reuse the resolved value instead: call ValidateAndResolveOcmUrl and assign the returned resolved URL to a variable (or overwrite e.hiveOcmUrl) and then pass that resolved URL into CreateConnectionWithUrl so you avoid redundant resolution while preserving the existing validation/error handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cmd/network/verification.go`:
- Around line 679-685: The current validation calls
utils.ValidateAndResolveOcmUrl(e.hiveOcmUrl) but discards the resolved URL,
causing a second resolution later in CreateConnectionWithUrl; capture and reuse
the resolved value instead: call ValidateAndResolveOcmUrl and assign the
returned resolved URL to a variable (or overwrite e.hiveOcmUrl) and then pass
that resolved URL into CreateConnectionWithUrl so you avoid redundant resolution
while preserving the existing validation/error handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 58a63297-f56e-434a-8f69-d353a139c2a0
📒 Files selected for processing (4)
cmd/network/verification.gocmd/network/verification_test.godocs/README.mddocs/osdctl_network_verify-egress.md
🚧 Files skipped from review as they are similar to previous changes (3)
- docs/osdctl_network_verify-egress.md
- cmd/network/verification_test.go
- docs/README.md
nephomaniac
force-pushed
the
SREP-3938-verify-egress-multi-env-ocm
branch
from
2c27d92 to
1b61b96
Compare
|
Looks like node failed to run pod for format job? |
|
@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 network verify-egresscommand to enable multi-environment OCM support for staging/integration testing.Changes
--hive-ocm-urloptional flag to network verify-egress commandvalidateInput()method to fail fast on invalid OCM URLsverification_test.goto verify early validation behaviorImplementation Details
When
--hive-ocm-urlis specified:utils.CreateConnectionWithUrl()utils.GetHiveClusterWithConn()to query hive from different OCM environmentutils.ValidateAndResolveOcmUrl()Testing
Testing Limitation
--hive-ocm-urlrequires a cluster configured with a cluster-wide proxy. No such clusters were available for testing in either staging or production environments. The implementation follows the same pattern as other commands in this series (SREP-3940, SREP-3941, SREP-3896) and has been verified through code review and unit tests.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