Add distribution validation with structured error handling #434
Conversation
- Add Distribution type with validation logic - Move validation logic to separate distribution.go file - Add proper error messages for invalid distribution/version combinations
There was a problem hiding this comment.
Pull Request Overview
This PR introduces distribution validation with structured error handling by centralizing distribution constants and adding validation logic for distribution/version combinations.
- Creates centralized distribution constants in
imagecustomizerapipackage - Adds validation logic to ensure only supported distribution/version combinations are used
- Removes duplicate constants from
distrohandler.goand updates references
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
toolkit/tools/pkg/imagecustomizerlib/distrohandler.go |
Removes duplicate distribution constants and updates references to use centralized ones |
toolkit/tools/imagecustomizerapi/distribution.go |
Adds centralized distribution constants and supported distribution/version mapping |
toolkit/tools/imagecreator/main.go |
Integrates distribution validation into main CLI flow with proper error handling |
toolkit/tools/imagecreator/distribution.go |
Implements distribution validation logic with structured error messages |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
toolkit/tools/imagecreator/main.go
Outdated
| log.Fatalf("invalid distribution arguments: %v", err) | ||
| } | ||
|
|
||
| err = imagecreatorlib.CreateImageWithConfigFile(ctx, cli.BuildDir, cli.ConfigFile, cli.RpmSources, |
There was a problem hiding this comment.
API validity checks should be done in imagecreatorlib. This will ensure imagecreatorlib is usable by itself.
There was a problem hiding this comment.
The validity check should actually occur in imagecreatorlib, instead of just moving the validation code into imagecreatorlib.
The CLI package should be almost entirely free of any logic.
toolkit/tools/imagecreator/main.go
Outdated
| ToolsTar string `name:"tools-file" help:"Path to tdnf worker tarball" required:""` | ||
| OutputImageFile string `name:"output-image-file" help:"Path to write the customized image to."` | ||
| OutputImageFormat string `name:"output-image-format" placeholder:"(vhd|vhd-fixed|vhdx|qcow2|raw)" help:"Format of output image." enum:"${imageformat}" default:""` | ||
| Distro string `name:"distro" help:"Target distribution for the image." enum:"azurelinux,fedora" default:"azurelinux"` |
There was a problem hiding this comment.
Do we ever expect a config file to be usable by more than 1 distro and/or version? If not, then it would probably make sense to remove the distro and distro-version and add equivalent fields into the config file instead.
There was a problem hiding this comment.
the same config file would work for fedora and azurelinux
toolkit/tools/imagecreator/main.go
Outdated
| log.Fatalf("invalid distribution arguments: %v", err) | ||
| } | ||
|
|
||
| err = imagecreatorlib.CreateImageWithConfigFile(ctx, cli.BuildDir, cli.ConfigFile, cli.RpmSources, |
There was a problem hiding this comment.
The validity check should actually occur in imagecreatorlib, instead of just moving the validation code into imagecreatorlib.
The CLI package should be almost entirely free of any logic.
| DistroNameFedora string = "fedora" | ||
| ) | ||
|
|
||
| func GetSupportedDistros() map[string][]string { |
There was a problem hiding this comment.
Image Customizer and Image Creator will likely have different support lists. So, it would be good to give this a name unique to Image Creator.
One option would be to create an imagecreatorapi package.
There was a problem hiding this comment.
That's a good point. for now, this applies to both tools.
There was a problem hiding this comment.
Fedora isn't supported in Image Customizer yet. Also, the GetSupportedDistros function is only called by the imagecreatorlib package.
| DistroNameFedora string = "fedora" | ||
| ) | ||
|
|
||
| func GetSupportedDistros() map[string][]string { |
There was a problem hiding this comment.
Fedora isn't supported in Image Customizer yet. Also, the GetSupportedDistros function is only called by the imagecreatorlib package.
4 participants
With wrong cli arguments for distro and distro-version we can now get a descriptive error message
2025/09/17 12:50:18 invalid distribution arguments: unsupported version "40" for distribution "azurelinux". Supported versions: 2.0, 3.0
Checklist