fix: make sources-files update sources#69
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates source preparation so that a component’s source-files configuration can append new entries into the Fedora/RHEL sources file, requiring both hash and hash-type and erroring on filename conflicts.
Changes:
- Refactors Fedora
sourcesparsing to expose a reusableParseSourcesFileand adds aFormatSourcesEntryhelper. - Extends source preparation to append configured
source-filesentries into the outputsourcesfile (with validation and conflict detection). - Updates/adds unit tests for the new formatting and sources-file update behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| internal/providers/sourceproviders/fedorasource/fedorasource.go | Introduces exported parsing/formatting helpers and refactors URL-building to use parsed entries. |
| internal/providers/sourceproviders/fedorasource/fedorasource_test.go | Updates hash type assertions and adds coverage for FormatSourcesEntry. |
| internal/app/azldev/core/sources/sourceprep.go | Adds sources file update step during PrepareSources, parsing existing entries and appending new ones from config. |
| internal/app/azldev/core/sources/sourceprep_test.go | Adds tests verifying sources file creation/appending and validation errors. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
fa1fd8a to
3d52ce6
Compare
|
|
||
| // HashTypeSHA256 represents the SHA-256 hash algorithm. | ||
| HashTypeSHA256 HashType = "sha256" | ||
| HashTypeSHA256 HashType = "SHA256" |
There was a problem hiding this comment.
Note: all current TOML configs use the upper case version, so this should not break them.
| "invalid 'uri' for source file %#q, component %#q; "+ | ||
| "URI %#q is missing a scheme (e.g. 'https://')", | ||
| filename, componentName, origin.Uri) | ||
| } |
There was a problem hiding this comment.
For 'download' origins, validation currently only checks that the URI has some scheme. Since source downloading uses the HTTP downloader (net/http), non-HTTP schemes (e.g. file://, ftp://) and even malformed HTTP(S) URLs with an empty host (e.g. https:///path) will pass config validation but fail later at runtime. Consider restricting schemes to http/https (and requiring a non-empty host for those schemes) so config errors are caught early with a clear message.
| } | |
| } | |
| if parsed.Scheme != "http" && parsed.Scheme != "https" { | |
| return fmt.Errorf( | |
| "invalid 'uri' for source file %#q, component %#q; "+ | |
| "URI %#q must use 'http' or 'https'", | |
| filename, componentName, origin.Uri) | |
| } | |
| if parsed.Host == "" { | |
| return fmt.Errorf( | |
| "invalid 'uri' for source file %#q, component %#q; "+ | |
| "URI %#q must include a host", | |
| filename, componentName, origin.Uri) | |
| } |
| if err := fileutils.ValidateFilename(ref.Filename); err != nil { | ||
| return nil, fmt.Errorf("invalid filename %#q in 'source-files' configuration:\n%w", ref.Filename, err) | ||
| } |
There was a problem hiding this comment.
FormatSourcesEntry writes the filename verbatim inside parentheses. Filenames containing a closing parenthesis ) (or newlines) will produce a 'sources' entry that ReadSourcesFileEntries cannot parse (its regex stops at )), and will likely break downstream tooling. Since ValidateFilename doesn’t reject these characters, consider adding an explicit validation in buildSourceEntries (or config validation) to reject filenames that can’t be represented in the modern sources-file format, with a clear error.
Adding a modification to how
source-filesconfig affects the outputsourcesfile:sourcesfile.source-filesentries must have their hash and hash type provided unless the user passes the--allow-no-hashesflag, which will then retrieve the sources and calculate the missing hashes.sourcesfile, it's considered an error. The user is told to either remove the original entry through an overlay (I'll add it in a separate PR, if it doesn't exist yet) or to re-name the file fromsource-files.I've also added config validation for the
source-filessection of the TOML and update the code to work only with the hash types defined in theHashType*constants.