feat: allow component definitions to merge across config files#296
feat: allow component definitions to merge across config files#296ddstreet wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the project config loader to allow multiple config files to define the same component name, merging component fields across files (similar to distro field-level merges) so later files can override earlier values.
Changes:
- Remove the “duplicate component” error and implement component-level merge during config load using
ComponentConfig.MergeUpdatesFrom. - Update loader unit tests to validate overriding, preservation of earlier fields, slice-append behavior, and
SourceConfigFilebehavior. - Update user documentation to describe component merge behavior across config files.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| internal/projectconfig/loader.go | Removes duplicate-component rejection and merges component definitions across files using MergeUpdatesFrom. |
| internal/projectconfig/loader_test.go | Reworks and adds tests to validate new merge semantics and ordering. |
| docs/user/explanation/config-system.md | Documents the updated merge rules for components across config files. |
| // Fill out fields not explicitly serialized. | ||
| component.Name = componentName | ||
| component.SourceConfigFile = loadedCfg | ||
|
|
||
| resolvedCfg.Components[componentName] = *(component.WithAbsolutePaths(loadedCfg.dir)) | ||
| resolved := component.WithAbsolutePaths(loadedCfg.dir) |
| // SourceConfigFile should reference the last file that contributed. | ||
| assert.Equal(t, "/project/sub", comp.SourceConfigFile.dir) | ||
| } |
| Component definitions are merged additively. If the same component name (e.g., `curl`) appears in multiple files, later files' non-empty fields override earlier ones. This allows splitting a component definition across files — for example, defining the component's general properties in one file and overriding specific details in another. | ||
|
|
||
| > **Note:** Slice fields (like `overlays`) are **appended**, not replaced, following the same merge behavior used by component configuration inheritance. |
| if existing, ok := resolvedCfg.Components[componentName]; ok { | ||
| err := existing.MergeUpdatesFrom(resolved) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to merge component %#q:\n%w", componentName, err) | ||
| } |
reubeno
left a comment
reubeno
left a comment
There was a problem hiding this comment.
Change generally looks righteous. One main question, may need input from Toni or Daniel to understand implications.
| } | ||
|
|
||
| // Preserve the latest source config file reference. | ||
| existing.SourceConfigFile = loadedCfg |
There was a problem hiding this comment.
The rest of this change looks fine to me. My main question is around the SourceConfigFile.
I think it had originally only been used for informational purposes, but I see that's now expanded a bit to usage within synthetic git history generation and presumably the rendering process.
@Tonisal-byte / @dmcilvaney -- what risk, if any, do you see of this path potentially shifting if multiple TOML files provide content that contribute to the resolved component object?
There was a problem hiding this comment.
These changes should be okay for the synthetic history generation, as long as every contributing TOML is in the same Git worktree. The only potential issue I see is that there are some overlays and source-file toml fields that may contain relative paths. Once an additional component definition is created the relative reference breaks. We'd have to go back in and normalize these to absolute paths.
There was a problem hiding this comment.
I'm not entirely clear if I need to change something to address this feedback?
|
|
||
| resolvedCfg.Components[componentName] = *(component.WithAbsolutePaths(loadedCfg.dir)) | ||
| resolved := component.WithAbsolutePaths(loadedCfg.dir) | ||
|
|
There was a problem hiding this comment.
Should normalize overlay-files field, suggested change:
for i := range resolved.OverlayFiles {
resolved.OverlayFiles[i] = makeAbsolute(loadedCfg.dir, resolved.OverlayFiles[i])
}
There was a problem hiding this comment.
@Tonisal-byte do we need to do the same for source-files's origin.script?
Remove the restriction that required each component name to be unique across all config files. Components now support field-level merge like distros: if the same component name appears in multiple files, later files' non-empty fields override earlier ones. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
119fe6d to
2177ebe
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
internal/projectconfig/loader.go:236
- Overwriting
existing.SourceConfigFileon every merge changes which directory is used to infer an upstream component’s “spec directory” (and thus where custom-source generation scripts are looked up). For example,internal/providers/sourceproviders/customsourceprovider.goderivesspecDirfromconfig.SourceConfigFile.Dir()for upstream components; if a later config file only overridesspec.upstream-commit, this code will still redirect script lookup to the later file’s directory, potentially breaking previously-working configs.
// Preserve the latest source config file reference.
existing.SourceConfigFile = loadedCfg
resolvedCfg.Components[componentName] = existing
docs/user/explanation/config-system.md:87
- The note says “Slice fields (like
overlays) are appended, not replaced”, but there is at least one slice field in component configs that is replaced when set:overlay-files(an explicit empty list disables inherited patterns). This wording is likely to mislead users into thinkingoverlay-files(and possibly other slice fields) always append across files.
> **Note:** Slice fields (like `overlays`) are **appended**, not replaced, following the same merge behavior used by component configuration inheritance.
| } | ||
|
|
||
| if existing, ok := resolvedCfg.Components[componentName]; ok { | ||
| err := existing.MergeUpdatesFrom(resolved) |
There was a problem hiding this comment.
@Tonisal-byte If I recall there is some logic to detect duplicate source customizations right? Do we need to deal with that?
| } | ||
|
|
||
| if existing, ok := resolvedCfg.Components[componentName]; ok { | ||
| err := existing.MergeUpdatesFrom(resolved) |
There was a problem hiding this comment.
Looking at MergeUpdatesFrom, I think it doesn't handle recursive maps well. We have append set for slices, but not for maps. I'd guess that setting stuff like packages.some-pkg.publish.rpm-channel = "foo" might be something we want to set separately from other package bits, and currently the packages.some-pkg map is last one wins. Could also just weaken the documentation.
|
Also, looking into why the python linter started flagging today. |
#297 should fix it. |
Remove the restriction that required each component name to be unique across all config files. Components now support field-level merge like distros: if the same component name appears in multiple files, later files' non-empty fields override earlier ones.