CNTRLPLANE-3843: feat: ho,cpo,hcco: expose console URL#9008
CNTRLPLANE-3843: feat: ho,cpo,hcco: expose console URL#9008stevekuznetsov wants to merge 2 commits into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
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:
📝 WalkthroughWalkthroughAdds validated Sequence Diagram(s)sequenceDiagram
participant GuestConsole as Guest Console resource
participant HCPStatus as HCP status controller
participant HCP as HostedControlPlane status
participant HCReconciler as HostedCluster reconciler
participant HC as HostedCluster status
GuestConsole->>HCPStatus: Console event
HCPStatus->>GuestConsole: Read console URL
HCPStatus->>HCP: Update consoleURL
HCReconciler->>HCP: Read consoleURL
HCReconciler->>HC: Update consoleURL
Suggested reviewers: 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
area/api
![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)
api/hypershift/v1beta1/hosted_controlplane.go (1)
363-369: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse consistent URL validation on both status fields.
Length constraints alone allow malformed non-empty values to pass the API schema.
api/hypershift/v1beta1/hosted_controlplane.go#L363-L369: add URL/CEL validation forHostedControlPlaneStatus.ConsoleURL.api/hypershift/v1beta1/hostedcluster_types.go#L2411-L2417: apply the identical validation toHostedClusterStatus.ConsoleURL.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/hypershift/v1beta1/hosted_controlplane.go` around lines 363 - 369, The ConsoleURL status fields currently validate only length and must also reject malformed non-empty URLs. Add the same URL/CEL validation markers to HostedControlPlaneStatus.ConsoleURL in api/hypershift/v1beta1/hosted_controlplane.go (lines 363-369) and HostedClusterStatus.ConsoleURL in api/hypershift/v1beta1/hostedcluster_types.go (lines 2411-2417), preserving the existing optional and length constraints.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@control-plane-operator/hostedclusterconfigoperator/controllers/hcpstatus/hcpstatus.go`:
- Around line 244-249: Update the Console retrieval branch around
h.hostedClusterClient.Get to clear hcp.Status.ConsoleURL when the resource is
NotFound, and return or requeue other retrieval errors so reconciliation does
not report success with stale status. Preserve assigning
console.Status.ConsoleURL on successful retrieval, using the controller’s
existing error classification and retry mechanism.
---
Nitpick comments:
In `@api/hypershift/v1beta1/hosted_controlplane.go`:
- Around line 363-369: The ConsoleURL status fields currently validate only
length and must also reject malformed non-empty URLs. Add the same URL/CEL
validation markers to HostedControlPlaneStatus.ConsoleURL in
api/hypershift/v1beta1/hosted_controlplane.go (lines 363-369) and
HostedClusterStatus.ConsoleURL in api/hypershift/v1beta1/hostedcluster_types.go
(lines 2411-2417), preserving the existing optional and length constraints.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: a56bc590-64a2-4275-9e93-1d49e9f8bf94
⛔ Files ignored due to path filters (38)
api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterUpdateAcceptRisks.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDC.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUpstreamParity.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/GCPPlatform.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/HCPEtcdBackup.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/HyperShiftOnlyDynamicResourceAllocation.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ImageStreamImportMode.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/KMSEncryptionProvider.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/OpenStack.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/TLSAdherence.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ClusterUpdateAcceptRisks.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDC.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDCWithUpstreamParity.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/GCPPlatform.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/HCPEtcdBackup.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/HyperShiftOnlyDynamicResourceAllocation.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ImageStreamImportMode.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/KMSEncryptionProvider.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/OpenStack.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/TLSAdherence.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**client/applyconfiguration/hypershift/v1beta1/hostedclusterstatus.gois excluded by!client/**client/applyconfiguration/hypershift/v1beta1/hostedcontrolplanestatus.gois excluded by!client/**cmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamldocs/content/reference/aggregated-docs.mdis excluded by!docs/content/reference/aggregated-docs.mddocs/content/reference/api.mdis excluded by!docs/content/reference/api.mdvendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hosted_controlplane.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hostedcluster_types.gois excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (5)
api/hypershift/v1beta1/hosted_controlplane.goapi/hypershift/v1beta1/hostedcluster_types.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/hcpstatus/hcpstatus.gohypershift-operator/controllers/hostedcluster/hostedcluster_controller.gohypershift-operator/controllers/hostedcluster/reconcile_legacy.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #9008 +/- ##
==========================================
+ Coverage 44.11% 44.15% +0.04%
==========================================
Files 772 772
Lines 96098 96345 +247
==========================================
+ Hits 42391 42544 +153
- Misses 50765 50855 +90
- Partials 2942 2946 +4
... and 16 files with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
/pipeline required |
|
Scheduling tests matching the |
cblecker
left a comment
cblecker
left a comment
There was a problem hiding this comment.
Test coverage: The new Console URL propagation path doesn't have unit test coverage in hcpstatus_test.go. The existing test cases cover analogous flows (Authentication propagation, Authentication missing). A couple of cases would be straightforward: Console present with a URL set, and Console missing.
Nit: The block comment at hostedcluster_controller.go:1128 and reconcile_legacy.go:808 reads // Set the Control Plane and OAuth endpoints URL but now also propagates ConsoleURL.
Test Resultse2e-aws
Failed TestsTotal failed tests: 8
... and 3 more failed tests e2e-aks
|
2230ccd to
55927c6
Compare
openshift-ci
Bot
added
the
area/testing
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
hypershift-operator/controllers/hostedcluster/reconcile_legacy.go (1)
810-820: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winClear the HostedCluster URL when the HCP is absent.
If a prior reconcile populated
hcluster.Status.ConsoleURLand the HCP GET now returnsNotFound, this guarded assignment is skipped and the stale URL remains exposed. Explicitly clearConsoleURLin thehcp == nilpath.Suggested fix
if hcp != nil { hcluster.Status.ControlPlaneEndpoint = hcp.Status.ControlPlaneEndpoint hcluster.Status.OAuthCallbackURLTemplate = hcp.Status.OAuthCallbackURLTemplate hcluster.Status.ConsoleURL = hcp.Status.ConsoleURL +} else { + hcluster.Status.ConsoleURL = "" }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@hypershift-operator/controllers/hostedcluster/reconcile_legacy.go` around lines 810 - 820, Update the HCP status propagation block in the reconcile flow to explicitly clear hcluster.Status.ConsoleURL when hcp is nil, while preserving the existing assignment from hcp.Status.ConsoleURL when the HCP exists.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@support/capabilities/hosted_control_plane_capabilities.go`:
- Around line 40-51: Add table-driven unit tests for IsConsoleCapabilityEnabled
covering nil capabilities, empty capabilities, an unrelated disabled capability,
and hyperv1.ConsoleCapability disabled; assert true for the first three cases
and false for the Console-disabled case.
In `@test/e2e/v2/tests/hosted_cluster_health_test.go`:
- Around line 203-215: Update the two MgmtClient.Get assertions in the
Eventually block to include diagnostic messages with each resource’s exact
namespace/name identity. Use the HostedControlPlane lookup’s hostedCluster.Name
and tc.ControlPlaneNamespace, and the HostedCluster lookup’s
ObjectKeyFromObject(hostedCluster) identity, while preserving the existing
Succeed expectations.
- Around line 194-196: Update the new It block in ValidateConsoleURLTest to
include the repository’s Console URL feature label, following the existing label
syntax and conventions. Ensure the test remains selectable through feature
filtering and add any required label to the surrounding Describe block per the
repository’s path-specific E2E conventions.
---
Outside diff comments:
In `@hypershift-operator/controllers/hostedcluster/reconcile_legacy.go`:
- Around line 810-820: Update the HCP status propagation block in the reconcile
flow to explicitly clear hcluster.Status.ConsoleURL when hcp is nil, while
preserving the existing assignment from hcp.Status.ConsoleURL when the HCP
exists.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 1d2365a0-e3a5-4211-8d84-1b6c2e862e82
⛔ Files ignored due to path filters (38)
api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterUpdateAcceptRisks.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDC.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUpstreamParity.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/GCPPlatform.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/HCPEtcdBackup.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/HyperShiftOnlyDynamicResourceAllocation.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ImageStreamImportMode.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/KMSEncryptionProvider.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/OpenStack.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/TLSAdherence.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ClusterUpdateAcceptRisks.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDC.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDCWithUpstreamParity.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/GCPPlatform.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/HCPEtcdBackup.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/HyperShiftOnlyDynamicResourceAllocation.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ImageStreamImportMode.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/KMSEncryptionProvider.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/OpenStack.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/TLSAdherence.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**client/applyconfiguration/hypershift/v1beta1/hostedclusterstatus.gois excluded by!client/**client/applyconfiguration/hypershift/v1beta1/hostedcontrolplanestatus.gois excluded by!client/**cmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamldocs/content/reference/aggregated-docs.mdis excluded by!docs/content/reference/aggregated-docs.mddocs/content/reference/api.mdis excluded by!docs/content/reference/api.mdvendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hosted_controlplane.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hostedcluster_types.gois excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (9)
api/hypershift/v1beta1/hosted_controlplane.goapi/hypershift/v1beta1/hostedcluster_types.gocontrol-plane-operator/hostedclusterconfigoperator/cmd.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/hcpstatus/hcpstatus.gocontrol-plane-operator/hostedclusterconfigoperator/operator/config.gohypershift-operator/controllers/hostedcluster/hostedcluster_controller.gohypershift-operator/controllers/hostedcluster/reconcile_legacy.gosupport/capabilities/hosted_control_plane_capabilities.gotest/e2e/v2/tests/hosted_cluster_health_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go
- api/hypershift/v1beta1/hosted_controlplane.go
- api/hypershift/v1beta1/hostedcluster_types.go
55927c6 to
e9b183b
Compare
|
Now I have the complete picture. Let me produce the final report. Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryThe PR adds 100 new lines across 5 files to expose the OpenShift console URL through the HCP status, but only 17 of those lines are covered by tests (16.66% patch coverage). The codecov/patch check fails because patch coverage is 27.45 percentage points below the target threshold of 44.11% (matching the base branch's overall project coverage). The codecov/project check fails because overall project coverage dropped from 44.11% to 44.08%. Four files contribute uncovered code: Root CauseThe PR introduces a new feature to expose the OpenShift console URL through the HostedControlPlane status, but the new code paths lack unit test coverage. Specifically:
The only file with full patch coverage is These are not infrastructure or flaky failures — they are legitimate coverage gaps in the newly added feature code. Recommendations
Evidence
|
deads2k
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@stevekuznetsov: This pull request references CNTRLPLANE-3843 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. 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. |
e9b183b to
93ae40f
Compare
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
7a05bcb to
08f5f8d
Compare
|
@stevekuznetsov I think your agent may have gone the opposite direction and removed the MaxLength everywhere as opposed to validating it? |
|
@cblecker I didn't realize we had some weird "linter" enforcing things like "all string fields in the API have a max length", so I removed it. Sigh. |
08f5f8d to
e012927
Compare
|
/pipeline required |
|
Scheduling tests matching the |
cblecker
left a comment
cblecker
left a comment
There was a problem hiding this comment.
ValidHostedClusterStatus needs version-gating and upgrade-context exclusions following the ConfigOperatorReconciliationSucceeded pattern at test/e2e/util/util.go:3126-3134. HCCO is delivered via the OCP payload — during upgrades the old HCCO won't set this condition, so the HC controller synthesizes Unknown while ExpectedHCConditions expects True.
In test/e2e/util/util.go around line 3135:
if IsLessThan(Version424) {
delete(expectedConditions, hyperv1.ValidHostedClusterStatus)
}
if upgradeContext != nil {
delete(expectedConditions, hyperv1.ValidHostedClusterStatus)
}In test/e2e/v2/tests/hosted_cluster_health_test.go around line 69 (v2 doesn't use upgrade context):
if e2eutil.IsLessThan(e2eutil.Version424) {
delete(expectedConditions, hyperv1.ValidHostedClusterStatus)
}A Version424 constant needs to be added to test/e2e/util/version.go.
a9c9b00 to
9366ce5
Compare
|
seems like an unrelated flake in unit: |
| hyperv1.ReconciliationSucceeded: metav1.ConditionTrue, | ||
| hyperv1.ConfigOperatorReconciliationSucceeded: metav1.ConditionTrue, | ||
| hyperv1.ValidHostedControlPlaneConfiguration: metav1.ConditionTrue, | ||
| hyperv1.ValidHostedClusterStatus: metav1.ConditionTrue, |
There was a problem hiding this comment.
The prior HCCO-set conditions all have version guards in ValidateHostedClusterConditionsTest (DataPlaneConnectionAvailable for <4.21, ControlPlaneConnectionAvailable for <4.22, ConfigOperatorReconciliationSucceeded for <4.23). Should ValidHostedClusterStatus get the same treatment? Without it, the upgrade e2e test would fail when the new test binary runs against a hosted cluster whose HCCO (from the old release image) doesn't set this condition yet.
There was a problem hiding this comment.
the file has a version guard already
Service operators would like to expose the console URL to users, and users control how the console URL actually gets routed by changing config in the cluster, so we need to read it from the cluster for the correct source of truth. Signed-off-by: Steve Kuznetsov <stekuznetsov@microsoft.com>
9366ce5 to
d77cdcc
Compare
|
/pipeline required |
|
Scheduling tests matching the |
hcpstatus.Setup was removed from controllerFuncs because its constructor now needs capabilities, but allControllers() builds its list by iterating controllerFuncs. This meant the hcpstatus controller name was never in o.Controllers, making the special-case in the Run loop unreachable and the controller never started. Signed-off-by: Cesar Wong <cewong@redhat.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
/pipeline required |
|
/test e2e-aws |
|
/test e2e-aws images okd-scos-images verify-deps |
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bryan-cox, cblecker, csrwng, stevekuznetsov The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/pipeline required |
|
Scheduling tests matching the |
|
@stevekuznetsov: The following test failed, say
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. |
7 participants
Service operators would like to expose the console URL to users, and users control how the console URL actually gets routed by changing config in the cluster, so we need to read it from the cluster for the correct source of truth.
Summary by CodeRabbit
New Features
Tests