Skip to content

OCPBUGS-78995: Azure Machines: Accept explicit image ID #10712

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.

Sign up for GitHub

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

Open
2uasimojo wants to merge 1 commit into
base: main
Choose a base branch
from
+7 −0
Open

OCPBUGS-78995: Azure Machines: Accept explicit image ID #10712

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions pkg/asset/machines/azure/azuremachines.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -399,6 +399,9 @@ func capzImage(osImage aztypes.OSImage, azEnv aztypes.CloudEnvironment, confiden
ThirdPartyImage: false,
},
}
case strings.HasPrefix(rhcosImg, "/subscriptions/"):
// An explicit non-marketplace resource path was supplied. Use it as is.
return &capz.Image{ID: &rhcosImg}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
case rhcosImg == "" && !confidentialVM:
// hive calls the machines function, but may pass an empty
// string for rhcos. In which case, allow MAO to choose default.
Expand Down
4 changes: 4 additions & 0 deletions pkg/asset/machines/azure/machines.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -458,5 +458,9 @@ func mapiImage(osImage azure.OSImage, azEnv azure.CloudEnvironment, confidential
// for MAPI, by removing the /subspcription/ prefix.
func trimSubscriptionPrefix(image string) string {
rgIndex := strings.Index(image, "/resourceGroups/")
// Allow invalid inputs to fail downstream
if rgIndex < 0 {
return image
}
Comment on lines +461 to +464

@coderabbitai coderabbitai Bot Jul 24, 2026 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Add regression tests for valid and malformed image IDs.

This change fixes a panic path, but the modified helper has no visible unit tests. Cover a valid subscription-scoped ID, an ID without /resourceGroups/, and an empty input.

As per coding guidelines, all Go code under pkg/ must have unit tests.

🤖 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 `@pkg/asset/machines/azure/machines.go` around lines 461 - 464, Add regression
tests for the image-ID helper surrounding the rgIndex validation, covering a
valid subscription-scoped ID, an ID missing “/resourceGroups/”, and empty input;
assert the expected image result and ensure malformed inputs do not panic. Place
the tests with the existing unit tests for the Azure machine helpers.

Source: Coding guidelines

@2uasimojo 2uasimojo Jul 24, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you point to an existing unit test that covers this function?

@coderabbitai coderabbitai Bot Jul 24, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo '== Azure machine-related Go tests =='
fd -t f -e go . pkg/asset/machines/azure | sort

echo
echo '== Direct trimSubscriptionPrefix calls =='
rg -n -C 3 '\btrimSubscriptionPrefix\s*\(' pkg/asset/machines/azure -g '*_test.go' || true

echo
echo '== mapiImage calls in tests (indirect coverage candidate) =='
rg -n -C 3 '\bmapiImage\s*\(' pkg/asset/machines/azure -g '*_test.go' || true

echo
echo '== Image ResourceID assertions in Azure machine tests =='
rg -n -C 3 '(ResourceID|resourceGroups)' pkg/asset/machines/azure -g '*_test.go' || true

Length of output: 1037

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo '== Direct helper coverage in all Go tests =='
rg -n -C 3 '\btrimSubscriptionPrefix\s*\(' -g '*_test.go' . || true

echo
echo '== Indirect mapiImage coverage in all Go tests =='
rg -n -C 3 '\bmapiImage\s*\(' -g '*_test.go' . || true

echo
echo '== Tests covering the package or Azure resource-group image IDs =='
rg -n -C 3 '(pkg/asset/machines/azure|/resourceGroups/|ResourceID)' -g '*_test.go' . || true

Length of output: 3942

@2uasimojo, no—there is no existing unit test covering trimSubscriptionPrefix, directly or through mapiImage.

pkg/asset/machines/azure/ currently has no *_test.go files, and a repository-wide search found no test invocation of either helper. The closest match is pkg/types/azure/platform_test.go:TestSetBaseDomain, but it tests unrelated DNS zone parsing.

My original comment should have stated this more clearly: the requested tests would be new regression coverage, not an extension of an existing helper test.

@2uasimojo 2uasimojo Jul 24, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, imma leave it alone then, unless a human feels otherwise.

@coderabbitai coderabbitai Bot Jul 24, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@2uasimojo, acknowledged. The panic fix is appropriately minimal; additional regression coverage can be added later if a human reviewer requests it.

@patrickdillon patrickdillon Jul 26, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the motivation for this (I.e. what is the input/case where we hit this) change?

Generally the image function seems like a really good case for unit tests and should be easy to add with claude

@coderabbitai coderabbitai Bot Jul 26, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

return image[rgIndex:]
}