OCPBUGS-78995: Azure: Default Marketplace image (for hive) - #10707
OCPBUGS-78995: Azure: Default Marketplace image (for hive)#107072uasimojo wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
@2uasimojo: This pull request references Jira Issue OCPBUGS-78995, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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. |
|
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 (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughChangesAzure image selection
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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.
Actionable comments posted: 1
🤖 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 `@pkg/asset/machines/azure/azuremachines.go`:
- Around line 403-422: Add table-driven tests covering the Azure
marketplace-image selection chain: test GetArchitecture, mapiImage/capzImage,
and GenerateMachines for Arm64 → 420-arm, AMD64 Gen2 → 420-v2, and AMD64
non-Gen2 → aro_420. Cover the affected flow across
pkg/asset/machines/azure/azuremachines.go (lines 403-422 and 66),
pkg/asset/installconfig/azure/capabilities.go (lines 51-58), and
pkg/asset/machines/azure/machines.go (lines 186 and 437-439); no production
changes are required unless needed to make these tests exercise the existing
mapping.
🪄 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: 7d98a00f-6276-4b2c-824b-0062d8917646
📒 Files selected for processing (3)
pkg/asset/installconfig/azure/capabilities.gopkg/asset/machines/azure/azuremachines.gopkg/asset/machines/azure/machines.go
In a path that _should_ only be reachable from hive's MachinePool
controller, if no RHCOS image reference was provided, we were defaulting
to an empty `capz.Image{}` and letting MAO/MCO figure out what to use.
This wasn't working very well.
Here we hardcode the oldest marketplace image in this path instead. This
should end up being forward compatible; and the in-cluster operators
should eventually be able to upgrade it as needed.
This is one part of a two-part solution. The other part entails hive
improving its discovery of gallery images and (VM and image)
HyperVGenerations, which will need to be in place when they vendor this
change in.
9ca5074 to
77cfd19
Compare
|
/jira refresh |
|
@2uasimojo: This pull request references Jira Issue OCPBUGS-78995, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
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. |
|
@2uasimojo: This pull request references Jira Issue OCPBUGS-78995, which is valid. 3 validation(s) were run on this bug
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. |
|
/retest |
|
@2uasimojo: all tests passed! 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. |
|
/hold I think this has the potential to regress some edge cases. Still pondering design possibilities. |
|
/close in favor of #10712. |
|
@2uasimojo: Closed this PR. 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 kubernetes-sigs/prow repository. |
|
@2uasimojo: This pull request references Jira Issue OCPBUGS-78995. The bug has been updated to no longer refer to the pull request using the external bug tracker. 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. |
In a path that should only be reachable from hive's MachinePool controller, if no RHCOS image reference was provided, we were defaulting to an empty
capz.Image{}and letting MAO/MCO figure out what to use. This wasn't working very well.Here we hardcode the oldest marketplace image in this path instead. This should end up being forward compatible; and the in-cluster operators should eventually be able to upgrade it as needed.
This is one part of a two-part solution. The other part entails hive improving its discovery of gallery images and (VM and image) HyperVGenerations, which will need to be in place when they vendor this change in.
Summary by CodeRabbit