-
Notifications
You must be signed in to change notification settings - Fork 95
Move aks jobs to their own platform to differentiate from standard azure #2912
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dgoodwin The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@dgoodwin: The following tests failed, say
Full PR test history. Your PR dashboard. 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 kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/hold hypershift is platform:aws, topology:external which is how aks is setup, platform:azure, topology:external |
| {"-rosa", "rosa"}, // Keep above AWS as many ROSA jobs also mention AWS | ||
| {"-aws", "aws"}, | ||
| {"-alibaba", "alibaba"}, | ||
| {"-azure-aro-hcp", "aro"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Look here for ARO, are you sure all AKS is ARO?
|
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
WalkthroughUpdated platform classification logic to recognize the -aks pattern as "aks" instead of "azure" across both the variant registry code and snapshot data, affecting platform inference for jobs containing -aks in their identifiers. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
pkg/variantregistry/ocp.go(1 hunks)pkg/variantregistry/snapshot.yaml(7 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
pkg/variantregistry/ocp.gopkg/variantregistry/snapshot.yaml
🔇 Additional comments (1)
pkg/variantregistry/snapshot.yaml (1)
378-378: I'm unable to access the repository to complete the verification due to a clone operation failure. However, I can assess the review comment based on the context provided.The original review comment raises a valid concern about completeness—specifically whether all -aks entries in a large snapshot file (20,000+ lines) have been updated. The comment presents 7 hunks across releases 4.19–4.21, but doesn't confirm that these represent the entire set of changes needed.
Since I cannot execute scripts to verify this concern, I'll provide the rewritten comment reflecting this limitation:
Verify that all -aks variant entries have been updated.
The PR shows 7 hunks updating Platform from "azure" to "aks" for -aks entries across releases 4.19–4.21. However, without access to search the full 20,000+ line snapshot.yaml file, I cannot confirm whether these hunks represent all -aks entries requiring updates or if any entries were missed. Please verify locally that there are no remaining -aks entries with Platform: azure by running:
rg -B 10 'Platform: azure' pkg/variantregistry/snapshot.yaml | rg '\-aks'This should return no results if all -aks entries have been correctly updated.
Also applies to: 20464-20464, 20487-20487, 21563-21563, 21586-21586, 22161-22161, 22184-22184
| {"-azure-aro-hcp", "aro"}, | ||
| {"-azure", "azure"}, | ||
| {"-aks", "azure"}, | ||
| {"-aks", "aks"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Critical: Move the -aks pattern before -azure to prevent misclassification.
The -aks pattern on line 873 is checked after -azure (line 872). Since pattern matching returns on first match, jobs containing both strings (e.g., periodic-ci-openshift-hypershift-...-e2e-azure-aks-... as seen on line 680) will match -azure first and be incorrectly categorized as "azure" instead of "aks". This defeats the purpose of this PR.
Apply this diff to fix the ordering:
{"-azure-aro-hcp", "aro"},
- {"-azure", "azure"},
{"-aks", "aks"},
+ {"-azure", "azure"},This follows the same pattern as -rosa being checked before -aws (lines 868-869) and -azure-aro-hcp before -azure (lines 871-872).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {"-aks", "aks"}, | |
| {"-azure-aro-hcp", "aro"}, | |
| {"-aks", "aks"}, | |
| {"-azure", "azure"}, |
🤖 Prompt for AI Agents
In pkg/variantregistry/ocp.go around line 873, the "-aks" pattern is currently
listed after "-azure" causing jobs that contain both to be misclassified as
"azure"; move the {"-aks", "aks"} entry to appear before the {"-azure", "azure"}
entry so "-aks" is checked first, keeping the existing list formatting and
ordering conventions (similar to how "-rosa" precedes "-aws" and
"-azure-aro-hcp" precedes "-azure").
No description provided.