aro-hcp: add tenant-level cluster profiles for prow jobs that span tenant-level permissions#5097
Conversation
These new cluster profiles will be used by periodic cleanup jobs in openshift/release to clean up orphaned app registrations with expired credentials across ARO-HCP tenants. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
WalkthroughAdded two new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
bennerv
changed the title
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: bennerv 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 |
There was a problem hiding this comment.
Pull request overview
Adds two new ARO-HCP tenant-level cluster profiles intended for periodic cleanup jobs (in openshift/release) to target orphaned app registrations with expired credentials.
Changes:
- Introduced
aro-hcp-red-hat-tenantandaro-hcp-msft-test-tenantcluster profile constants. - Registered the new profiles in the global
ClusterProfiles()list. - Added
ClusterType()andLeaseType()mappings for the new profiles.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| case ClusterProfileAROHCPRedHatTenant: | ||
| return "aro-hcp-red-hat-tenant" | ||
| case ClusterProfileAROHCPMsftTestTenant: | ||
| return "aro-hcp-msft-test-tenant" |
There was a problem hiding this comment.
New ARO-HCP tenant profiles introduce new ClusterType values ("aro-hcp-red-hat-tenant" and "aro-hcp-msft-test-tenant"), but LeaseTypeFromClusterType’s allowlist (later in this file) does not include them. As a result, callers that convert from cluster-type -> lease-type will get invalid cluster type for these new profiles. Please add these two cluster types to the allowlist (or otherwise make the mapping handle them).
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/api/types.go`:
- Around line 2081-2084: LeaseTypeFromClusterType() currently rejects the two
new cluster constants ClusterProfileAROHCPRedHatTenant and
ClusterProfileAROHCPMsftTestTenant that were added in the switch (which causes
"invalid cluster type" at runtime); update LeaseTypeFromClusterType to include
case branches for ClusterProfileAROHCPRedHatTenant and
ClusterProfileAROHCPMsftTestTenant and return the appropriate LeaseType values
(use the same LeaseType used for other ARO HCP-related cluster cases), ensuring
the function's switch covers these new constants and the default error remains
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| case ClusterProfileAROHCPRedHatTenant: | ||
| return "aro-hcp-red-hat-tenant" | ||
| case ClusterProfileAROHCPMsftTestTenant: | ||
| return "aro-hcp-msft-test-tenant" |
There was a problem hiding this comment.
Add the new ARO HCP tenant cluster types to LeaseTypeFromClusterType().
Lines 2081-2084 add new cluster type outputs, but LeaseTypeFromClusterType() still rejects both values. That creates an internal mapping inconsistency and can surface as invalid cluster type at runtime in conversion paths.
Suggested fix
--- a/pkg/api/types.go
+++ b/pkg/api/types.go
@@
- "metal-redhat-gs", "aro-hcp-int", "aro-hcp-stg", "aro-hcp-prod", "aro-hcp-dev", "rosa-regional-platform-int", "hyperfleet-e2e",
+ "metal-redhat-gs", "aro-hcp-int", "aro-hcp-stg", "aro-hcp-prod", "aro-hcp-dev", "aro-hcp-red-hat-tenant", "aro-hcp-msft-test-tenant", "rosa-regional-platform-int", "hyperfleet-e2e",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 `@pkg/api/types.go` around lines 2081 - 2084, LeaseTypeFromClusterType()
currently rejects the two new cluster constants ClusterProfileAROHCPRedHatTenant
and ClusterProfileAROHCPMsftTestTenant that were added in the switch (which
causes "invalid cluster type" at runtime); update LeaseTypeFromClusterType to
include case branches for ClusterProfileAROHCPRedHatTenant and
ClusterProfileAROHCPMsftTestTenant and return the appropriate LeaseType values
(use the same LeaseType used for other ARO HCP-related cluster cases), ensuring
the function's switch covers these new constants and the default error remains
unchanged.
…ions Add new cluster profiles (aro-hcp-red-hat-tenant, aro-hcp-msft-test-tenant) and a periodic job to clean up orphaned app registrations with expired credentials left by e2e test runs. Depends on openshift/ci-tools#5097 for cluster profile registration. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@bennerv: The following tests 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. |
These new cluster profiles will be used by periodic cleanup jobs in openshift/release to clean up orphaned app registrations with expired credentials across ARO-HCP tenants.
These are tenant-level cluster profiles for ARO HCP as opposed to subscription-scoped / environment specific ones.