ARO-26764: promote OCM label prefix and validation skip reason to API package#8577
ARO-26764: promote OCM label prefix and validation skip reason to API package#8577tuxerrante wants to merge 11 commits into
Conversation
Reduce self-induced HostedCluster reconciles by filtering the primary watch to spec changes and explicit metadata triggers that already affect reconciliation. Preserve mirrored annotation and label behavior with focused falsification tests so stale queued requests still reconcile the latest HostedCluster state. Signed-off-by: Alessandro Affinito <aaffinit@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code) Co-authored-by: Cursor <cursoragent@cursor.com>
Track when release image validation was skipped so same-generation annotation changes can invalidate a cached True ValidReleaseImage condition. Add a focused regression test matrix covering skip annotation add and remove transitions without a generation bump. Signed-off-by: Alessandro Affinito <aaffinit@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code) Co-authored-by: Cursor <cursoragent@cursor.com>
Update the new HostedCluster predicate code to use the k8sutil-scoped annotation constants introduced on current main so the branch verifies cleanly after rebasing. Co-authored-by: Cursor <cursoragent@cursor.com>
Keep HostedControlPlane label mirroring aligned with the shared actionable label prefix list so the watch predicate and reconciliation logic stay in sync. Co-authored-by: Cursor <cursoragent@cursor.com>
Replace the dynamic platform override subtest titles with explicit static names so the HostedCluster predicate tests satisfy deterministic naming checks without changing the existing assertion style used in this package. Signed-off-by: Alessandro Affinito <aaffinit@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code) Co-authored-by: Cursor <cursoragent@cursor.com>
…neric names annotationPrefixChanged and prefixedAnnotations are used for both annotations and labels. Rename to prefixedKeyChanged and prefixedEntries with generic parameter names to avoid reader confusion when the functions are called with label maps. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…on is present Guard the validateReleaseImage call so it is not invoked when the skip annotation is set. Previously the call ran but its result was discarded by the switch branch, relying on an internal short-circuit. Making the guard explicit avoids unnecessary work and makes intent clearer. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace the local ptrToTime helper with ptr.To from k8s.io/utils/ptr, which is already a dependency in this package. Rename the predicate local variable to pred so it does not shadow the controller-runtime predicate package import. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Move the hardcoded "api.openshift.com/" label prefix to the API package as OCMLabelPrefix so consumers outside the controller (ARO-HCP, OCM) have a single source of truth for the prefix used to mirror labels from HostedCluster to HostedControlPlane. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…I package Move releaseImageValidationSkippedReason from a controller-private constant to the API package as ReleaseImageValidationSkippedReason, alongside the existing AsExpectedReason and InvalidImageReason. This allows external consumers (ARO-HCP, OCM) to inspect the ValidReleaseImage condition reason programmatically without hardcoding the string. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
@tuxerrante: This pull request references ARO-26764 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 enhancement 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. |
📝 WalkthroughWalkthroughThis PR refactors HostedCluster reconciliation to use a predicate-driven architecture and configurable metadata mirroring. It adds a new Suggested reviewers
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (10 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
openshift-ci
Bot
added
area/api
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: tuxerrante 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 |
|
I now have all the information needed. The root cause is clear. The last two commits on the PR use the prefix The gitlint configuration allows these types: The two failing commits use Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryThe gitlint CI check failed because the two most recent commits ( Root CauseThe two failing commits use [contrib-title-conventional-commits]
types = fix,feat,chore,docs,style,refactor,perf,test,revert,ci,buildThe gitlint tool enforces the This is purely a commit message formatting issue — not a code or test problem. The other 9 commits in this PR ( RecommendationsOption 1 (Recommended): Reword the two failing commit messages to use an allowed type that accurately describes the change. Since these commits move existing constants from a controller-private scope to the public API package,
Use Option 2: If the team wants types = fix,feat,chore,docs,style,refactor,perf,test,revert,ci,build,apiThis would require a separate PR and team consensus. Evidence
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
hypershift-operator/controllers/hostedcluster/hostedcluster_predicates_test.go (1)
299-305: ⚡ Quick winUse
hyperv1.OCMLabelPrefixin test label keys instead of hardcoded prefix.This avoids drift with the newly promoted API constant and keeps tests aligned with controller behavior.
As per coding guidelines, "`**/*.go`: Code changes must use golang common kubernetes patterns and best practices."Suggested patch
- hc.Labels = map[string]string{"api.openshift.com/id": "old"} + hc.Labels = map[string]string{hyperv1.OCMLabelPrefix + "id": "old"} return hc }(), newHC: func() *hyperv1.HostedCluster { hc := newHostedClusterForPredicateTests(1, nil) - hc.Labels = map[string]string{"api.openshift.com/id": "new"} + hc.Labels = map[string]string{hyperv1.OCMLabelPrefix + "id": "new"} return hc }(), @@ - hc.Labels = map[string]string{"api.openshift.com/id": "old"} + hc.Labels = map[string]string{hyperv1.OCMLabelPrefix + "id": "old"} return hc }(),Also applies to: 313-314
🤖 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/hostedcluster_predicates_test.go` around lines 299 - 305, Replace the hardcoded label prefix "api.openshift.com" used in the test setup with the shared constant hyperv1.OCMLabelPrefix so tests track the promoted API constant; update both instances where labels are set (inside the anonymous newHC functions that call newHostedClusterForPredicateTests and assign hc.Labels) to use hyperv1.OCMLabelPrefix + "/id" as the key instead of the literal string.hypershift-operator/controllers/hostedcluster/hostedcluster_hcp_labels_test.go (1)
27-27: ⚡ Quick winUse the promoted API prefix constant instead of hardcoded
"api.openshift.com/".This test should derive the actionable label key from
hyperv1.OCMLabelPrefixso it stays aligned with the API contract and avoids drift.As per coding guidelines, "Design APIs contract-first following OpenShift and Kubernetes best practices."Proposed change
Labels: map[string]string{ - "api.openshift.com/id": "stale", + hyperv1.OCMLabelPrefix + "id": "stale", "other": "keep", }, }, } @@ - if _, exists := hcp.Labels["api.openshift.com/id"]; exists { + if _, exists := hcp.Labels[hyperv1.OCMLabelPrefix+"id"]; exists { t.Fatal("expected stale api.openshift.com label to be removed from hosted control plane") }Also applies to: 39-39
🤖 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/hostedcluster_hcp_labels_test.go` at line 27, The test currently uses the hardcoded label key "api.openshift.com/id" (appearing twice) which can drift from the promoted API prefix; replace both occurrences by deriving the label key from the promoted constant hyperv1.OCMLabelPrefix (e.g. build the actionable key by appending "id" to hyperv1.OCMLabelPrefix) so the test uses the official prefix; update any assertions or map keys in hostedcluster_hcp_labels_test.go (the places that reference "api.openshift.com/id") to use the constructed key instead.
🤖 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
`@hypershift-operator/controllers/hostedcluster/hostedcluster_hcp_labels_test.go`:
- Line 27: The test currently uses the hardcoded label key
"api.openshift.com/id" (appearing twice) which can drift from the promoted API
prefix; replace both occurrences by deriving the label key from the promoted
constant hyperv1.OCMLabelPrefix (e.g. build the actionable key by appending "id"
to hyperv1.OCMLabelPrefix) so the test uses the official prefix; update any
assertions or map keys in hostedcluster_hcp_labels_test.go (the places that
reference "api.openshift.com/id") to use the constructed key instead.
In
`@hypershift-operator/controllers/hostedcluster/hostedcluster_predicates_test.go`:
- Around line 299-305: Replace the hardcoded label prefix "api.openshift.com"
used in the test setup with the shared constant hyperv1.OCMLabelPrefix so tests
track the promoted API constant; update both instances where labels are set
(inside the anonymous newHC functions that call
newHostedClusterForPredicateTests and assign hc.Labels) to use
hyperv1.OCMLabelPrefix + "/id" as the key instead of the literal string.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: bbbbec7b-58be-4859-91c9-5f8fe3e708e3
⛔ Files ignored due to path filters (2)
vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hostedcluster_conditions.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hostedcluster_types.gois excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (8)
api/hypershift/v1beta1/hostedcluster_conditions.goapi/hypershift/v1beta1/hostedcluster_types.gohypershift-operator/controllers/hostedcluster/hostedcluster_controller.gohypershift-operator/controllers/hostedcluster/hostedcluster_hcp_labels_test.gohypershift-operator/controllers/hostedcluster/hostedcluster_predicates.gohypershift-operator/controllers/hostedcluster/hostedcluster_predicates_test.gohypershift-operator/controllers/hostedcluster/hostedcluster_reconciliation_condition_test.gohypershift-operator/controllers/hostedcluster/hostedcluster_release_image_validation_test.go
|
Folded into #8424 directly — the 2 API promotion commits are now part of the main PR. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #8577 +/- ##
==========================================
+ Coverage 37.39% 40.42% +3.02%
==========================================
Files 751 756 +5
Lines 91806 93277 +1471
==========================================
+ Hits 34333 37708 +3375
+ Misses 54838 52866 -1972
- Partials 2635 2703 +68
... and 73 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:
|
2 participants
What this PR does / why we need it:
Follow-up to #8424 (review feedback from @jparrill).
Promotes two controller-private constants to the API package so that external consumers (ARO-HCP, OCM) can reference them programmatically instead of hardcoding strings:
OCMLabelPrefix("api.openshift.com/") — the label key prefix used by OCM to tag HostedCluster resources, mirrored to HostedControlPlane. Previously a hardcoded string in the predicate code; now lives alongsideLimitedSupportLabelinapi/hypershift/v1beta1/hostedcluster_types.go.ReleaseImageValidationSkippedReason("ReleaseImageValidationSkipped") — the condition reason set onValidReleaseImagewhen the skip annotation is present. Previously a controller-private constant; now lives alongsideAsExpectedReasonandInvalidImageReasoninapi/hypershift/v1beta1/hostedcluster_conditions.go.Which issue(s) this PR fixes:
Fixes sub-tasks of ARO-26764:
Notes:
make hypershift-apiCRD generation step has a pre-existing failure (Hit an unsupported type invalid type) unrelated to these changes — it reproduces on bothmainand ARO-26764: narrow HostedCluster primary watch to actionable metadata updates #8424's branchapi-linthas a pre-existingazure.gokubeapilinter finding also unrelated to these changesTest plan:
go vet— cleango test -race— all pass (hostedcluster package and subpackages)make lint— clean (the api-lint finding is pre-existing in azure.go, not from these changes)Checklist:
Summary by CodeRabbit
Release Notes
New Features
Tests