mantle/azure: Prepare to upload to Fedora Community Gallery#4500
mantle/azure: Prepare to upload to Fedora Community Gallery#4500marmijo wants to merge 6 commits intocoreos:mainfrom
Conversation
This was useful for a time, but now that we'll be uploading to shared community galleries, we should remove the functionality so we don't delete entire galleries by mistake.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the Azure API in mantle to support uploading images to the Fedora Community Shared Image Gallery. The changes include adding support for image offers, using a deterministic versioning scheme based on the CoreOS version, and adding required tags to the gallery images. The code is also updated to be more careful by checking for existing resources before creating them and removing the functionality to delete an entire gallery.
My review focuses on improving code maintainability and adhering to best practices. I've suggested refactoring some duplicated error-handling logic into a helper function and using returned errors instead of os.Exit in a command handler. I also found a minor typo in a comment.
marmijo
force-pushed
the
azure-gallery-updates
branch
from
d581583 to
d891aaf
Compare
Pass the full FCOS version through to the Azure gallery image creation so ore can derive an Azure-compatible gallery image version. Azure gallery image versions must be in x.y.z form, while FCOS versions include an additional "stream" component. The full FCOS version is now provided and truncated before creating the image version. See: coreos/coreos-assembler#4500
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request reworks the mantle Azure API to facilitate uploading images to the Fedora Community Shared Image Gallery. Key changes include adding flags for Azure offer and CoreOS version, making resource creation more idempotent by checking for existence before creation, and adjusting image identifiers for the gallery. The code is well-structured and aligns with the PR's objectives. I've included a couple of suggestions to improve error handling in the CLI and to follow Go best practices for context propagation, which will enhance maintainability and robustness.
Pass the full FCOS version through to the Azure gallery image creation so ore can derive an Azure-compatible gallery image version. Azure gallery image versions must be in x.y.z form, while FCOS versions include an additional "stream" component. The full FCOS version is now provided and truncated before creating the image version. See: coreos/coreos-assembler#4500
marmijo
force-pushed
the
azure-gallery-updates
branch
from
d891aaf to
91dfb49
Compare
dustymabe
left a comment
dustymabe
left a comment
There was a problem hiding this comment.
code LGTM.
the suggestions are purely cosmetic (naming and such) and optional.
| sv(&blobUrl, "image-blob", "", "source blob url") | ||
| sv(&resourceGroup, "resource-group", "kola", "resource group name") | ||
| sv(&architecture, "arch", "", "The target architecture for the image") | ||
| sv(&galleryProfile, "gallery-profile", "", "The azure image gallery profile") |
There was a problem hiding this comment.
OK. Nothing wrong with the code. This is just a naming thing.
| sv(&galleryProfile, "gallery-profile", "", "The azure image gallery profile") | |
| sv(&coreos-gallery-policy, "coreos-gallery-policy", "", "The CoreOS specific profile/policy to apply to the image on upload") |
I think right now the way things are presented to the user the user could think azure image gallery profile is actually an azure construct. It's not, this is CoreOS/Fedora Specific. The suggestion abovve is an attempt to try to improve the clarity here.
There was a problem hiding this comment.
I'm not tied to the suggested name above either.
There was a problem hiding this comment.
That makes a lot of sense. I'll change this to --coreos-gallery-profile.
| Architecture: &azureArch, | ||
| }, | ||
| }, nil) | ||
| profileCfg, err := a.resolveGalleryProfile(galleryProfile, version) |
There was a problem hiding this comment.
at least in its current form this function is just setting the version and tags so it might be easier to read if it just set those instead of returning a GalleryProfileConfig
| profileCfg, err := a.resolveGalleryProfile(galleryProfile, version) | |
| version, tags, err := a.resolveGalleryProfile(galleryProfile, version) |
totally optional.
There was a problem hiding this comment.
I went with the struct mostly to allow us to easily expand the profile handling in the future.
Right now it only operates on version on tags so returning those would work too. I kept GalleryProfileConfig because if we add more profile-specific behavior later, I think it will be cleaner to extend a single return type. Additional metadata or other per-gallery overrides would fit easily in there like Offer, SKU, and Provider, etc.
Thanks! I added that 4th commit to this PR to rework the image version/definition deletion. That commit also removes the deletion of the managed image that was being used as the source for the gallery image. So in it's current form, we'd have managed images accumulating with no garbage collection. I’m testing an SDK upgrade that would let us use the uploaded blob directly as the source for the gallery image version instead of the managed image as source. If that works cleanly and the change stays reasonably small, I’d like to include it in this PR so we can drop the managed image step entirely. EDIT: added a link to the "managed image step" |
Support gallery profiles for Azure gallery image creation so profile-specific version and tagging loggic can be handled in one place. The new `fedora-community` profile truncates FCOS versions to Azure's required x.y.z gallery image vresion format[1] and applies the Fedora community gallery tags[2]. With no profile selected, the version is used as-is and no extra tags are added. This keeps the Fedora-specific behavior explicit while leaving room for additional gallery profiles in the future. [1]: coreos/fedora-coreos-tracker#148 (comment) [2]: coreos/fedora-coreos-tracker#148 (comment)
Check for an existing gallery and image definition before creating it and reuse it if present. Also check for an existing gallery image version and fail if it already exists instead creating it.
Change Azure gallery deletion to operate on a single gallery image version instead of removing all versions and deleting the image definition unconditionally. Require `--version` for `delete-gallery-image`, apply gallery-profile version handling when resolving the version to delete, and treat a missing version as a successful no-op. Also add `--delete-definition` to optionally remove the gallery image definition after the version delete, letting Azure reject the request if other versions still exist in the definition.
refresh Go modules for azure-sdk-for-go armcompute/v4 upgrade. Append `/v4 v4.2.1` to `armcompute` in go.mod and update all 5 imports of `armcompute` in `mantle/platform/api/azure`. ``` go get github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v4 go mod vendor go mod tidy ```
Now that the `armcompute` libraries in the azure-sdk-for-go have been updated, we can stop creating an intermediate managed image before creating the gallery image version, and just use the blob directly as the source for the gallery image version. Resolve the storage account resource ID from the blob URL to use when creating the image from blob source, as required by the newer Azure Compute API.
marmijo
force-pushed
the
azure-gallery-updates
branch
from
d9d1428 to
bbae0a9
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates the Azure SDK dependency to version 4.2.1 and introduces several enhancements to the Azure gallery image management. Key changes include the addition of new CLI flags for gallery profiles and image versions, improved error handling for gallery resource creation, and the refactoring of Azure API client initialization to use a client factory. My feedback focuses on improving code maintainability by suggesting the extraction of 'get-or-create' logic into helper methods and recommending that CLI command functions return errors instead of using os.Exit(1).
Define the new coreos gallery profile config item in config.yaml and pass it to the Azure gallery image creation. Also pass the full FCOS version through so ore can derive an Azure-compatible gallery image version based on the gallery profile. Azure gallery image versions must be in x.y.z form, while FCOS versions include an additional "stream" component. The full FCOS version is now provided and truncated before creating the image version since we are using the "fedora-community" coreos gallery profile. See: coreos/coreos-assembler#4500
Define the new coreos gallery profile config item in config.yaml and pass it to the Azure gallery image creation. Also pass the full FCOS version through so ore can derive an Azure-compatible gallery image version based on the gallery profile. Azure gallery image versions must be in x.y.z form, while FCOS versions include an additional "stream" component. The full FCOS version is now provided and truncated before creating the image version since we are using the "fedora-community" coreos gallery profile. See: coreos/coreos-assembler#4500
Define the new coreos gallery profile config item in config.yaml and pass it to the Azure gallery image creation. Also pass the full FCOS version through so ore can derive an Azure-compatible gallery image version based on the gallery profile. Azure gallery image versions must be in x.y.z form, while FCOS versions include an additional "stream" component. The full FCOS version is now provided and truncated before creating the image version since we are using the "fedora-community" coreos gallery profile. See: coreos/coreos-assembler#4500
Define the new coreos gallery profile config item in config.yaml and pass it to the Azure gallery image creation. Also pass the full FCOS version through so ore can derive an Azure-compatible gallery image version based on the gallery profile. Azure gallery image versions must be in x.y.z form, while FCOS versions include an additional "stream" component. The full FCOS version is now provided and truncated before creating the image version since we are using the "fedora-community" coreos gallery profile. See: coreos/coreos-assembler#4500
2 participants
Rework the mantle Azure API to so we can start uploading to the Fedora Community Shared Image Gallery. See individual commits.
See:
Follow-on to #4109
This includes breaking changes to the pipeline, so we'll need coreos/fedora-coreos-pipeline#1323 to merge at the same time.