feat(HCCO): stop reconciling over componentRoutes and appsDomain on guest Ingress config#8471
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe 🚥 Pre-merge checks | ✅ 10 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: joshbranham 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
area/control-plane-operator
joshbranham
force-pushed
the
OCPSTRAT-2884-preserve-ingress-guest-fields
branch
from
6ac0288 to
efef27a
Compare
joshbranham
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
support/globalconfig/ingress_test.go (1)
250-349: ⚡ Quick winAdd explicit “guest empty, HCP populated” cases for preserved fields.
Please add two cases to lock in that
ComponentRoutesandAppsDomainremain guest-owned even when only HCP sets them. This makes the new contract unambiguous and prevents accidental regression.✅ Suggested test additions
+ { + name: "When guest cluster has no componentRoutes and HCP has componentRoutes it should keep componentRoutes nil", + inputIngressConfig: IngressConfig(), + inputHCP: &hyperv1.HostedControlPlane{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster"}, + Spec: hyperv1.HostedControlPlaneSpec{ + DNS: hyperv1.DNSSpec{BaseDomain: "example.com"}, + Configuration: &hyperv1.ClusterConfiguration{ + Ingress: &configv1.IngressSpec{ + Domain: "apps.cluster.example.com", + ComponentRoutes: []configv1.ComponentRouteSpec{{ + Namespace: "openshift-console", + Name: "console", + Hostname: "hcp-console.example.com", + }}, + }, + }, + }, + }, + expectedIngressConfig: &configv1.Ingress{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster"}, + Spec: configv1.IngressSpec{ + Domain: "apps.cluster.example.com", + }, + }, + }, + { + name: "When guest cluster has no appsDomain and HCP has appsDomain it should keep appsDomain empty", + inputIngressConfig: IngressConfig(), + inputHCP: &hyperv1.HostedControlPlane{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster"}, + Spec: hyperv1.HostedControlPlaneSpec{ + DNS: hyperv1.DNSSpec{BaseDomain: "example.com"}, + Configuration: &hyperv1.ClusterConfiguration{ + Ingress: &configv1.IngressSpec{ + Domain: "apps.cluster.example.com", + AppsDomain: "hcp-apps.example.com", + }, + }, + }, + }, + expectedIngressConfig: &configv1.Ingress{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster"}, + Spec: configv1.IngressSpec{ + Domain: "apps.cluster.example.com", + }, + }, + },🤖 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 `@support/globalconfig/ingress_test.go` around lines 250 - 349, Add two table-driven test cases in ingress_test.go mirroring the existing patterns that assert guest-owned fields remain when HCP alone populates them: (1) a case where inputIngressConfig has nil/empty Spec.ComponentRoutes and inputHCP.Spec.Configuration.Ingress has ComponentRoutes populated, and expectedIngressConfig must keep ComponentRoutes nil (verify ComponentRoutes is preserved from guest); (2) a case where inputIngressConfig has empty Spec.AppsDomain and inputHCP.Spec.Configuration.Ingress.AppsDomain is set, and expectedIngressConfig must keep AppsDomain empty. Use the same structure and names as other tests (IngressConfig(), &hyperv1.HostedControlPlane{...}, expectedIngressConfig: &configv1.Ingress{...}) so the reconciliation behavior for ComponentRoutes and AppsDomain is explicitly locked in.
🤖 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.
Nitpick comments:
In `@support/globalconfig/ingress_test.go`:
- Around line 250-349: Add two table-driven test cases in ingress_test.go
mirroring the existing patterns that assert guest-owned fields remain when HCP
alone populates them: (1) a case where inputIngressConfig has nil/empty
Spec.ComponentRoutes and inputHCP.Spec.Configuration.Ingress has ComponentRoutes
populated, and expectedIngressConfig must keep ComponentRoutes nil (verify
ComponentRoutes is preserved from guest); (2) a case where inputIngressConfig
has empty Spec.AppsDomain and inputHCP.Spec.Configuration.Ingress.AppsDomain is
set, and expectedIngressConfig must keep AppsDomain empty. Use the same
structure and names as other tests (IngressConfig(),
&hyperv1.HostedControlPlane{...}, expectedIngressConfig: &configv1.Ingress{...})
so the reconciliation behavior for ComponentRoutes and AppsDomain is explicitly
locked in.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 7c807022-16b6-4b3b-b95a-b30385432b50
📒 Files selected for processing (2)
support/globalconfig/ingress.gosupport/globalconfig/ingress_test.go
…uest Ingress config The HCCO was overwriting the entire Ingress.Spec on every reconciliation, which prevented customers from setting componentRoutes or appsDomain directly on the guest cluster. Preserve these fields so the Ingress Operator can act on customer-configured custom routes (e.g. Console, Downloads) and alternative apps domains. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
joshbranham
force-pushed
the
OCPSTRAT-2884-preserve-ingress-guest-fields
branch
from
efef27a to
7b2d435
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8471 +/- ##
==========================================
+ Coverage 37.53% 37.55% +0.02%
==========================================
Files 751 751
Lines 92026 92035 +9
==========================================
+ Hits 34544 34568 +24
+ Misses 54841 54825 -16
- Partials 2641 2642 +1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
Now I have the complete picture. Let me compile the final report. Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryThis is not a code coverage problem introduced by the PR. Codecov itself confirms "All modified and coverable lines are covered by tests" with 100% coverage on changed lines. The failure is caused by 2 of 5 unit test shards ( Root CauseThe root cause is a GitHub Actions runner infrastructure issue combined with a Codecov race condition:
Recommendations
Evidence
|
1 participant
What this PR does / why we need it:
The HCCO was overwriting the entire Ingress.Spec on every reconciliation, which prevented customers from setting componentRoutes or appsDomain directly on the guest cluster. Preserve these fields so the Ingress Operator can act on customer-configured custom routes (e.g. Console, Downloads) and alternative apps domains.
Which issue(s) this PR fixes:
Fixes
Special notes for your reviewer:
Checklist:
Summary by CodeRabbit
Bug Fixes
Tests