fix(overlays): expand overlay files after config resolution#256
Open
liunan-ms wants to merge 1 commit into
Open
fix(overlays): expand overlay files after config resolution#256liunan-ms wants to merge 1 commit into
liunan-ms wants to merge 1 commit into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request moves overlay-files glob expansion from raw config-file loading to the component resolution phase, enabling overlay-files to be inherited via default-component-config (project/distro/component-group) and resolved relative to each concrete component (or spec directory for spec-discovered components).
Changes:
- Expand and validate per-file overlay documents during component resolution (
ExpandResolvedOverlayFiles), then clearOverlayFilesto prevent double expansion. - Adjust overlay-file glob semantics: patterns with no matches are treated as a no-op instead of an error, and invalid overlays are validated with clearer per-overlay context.
- Update inheritance behavior so a non-empty
overlay-fileslist overrides inherited values, plus update schema/snapshots/docs/tests to reflect the new behavior.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| schemas/azldev.schema.json | Updates schema description to reflect post-resolution overlay-file expansion and spec-dir-relative behavior. |
| scenario/snapshots/TestSnapshotsContainer_config_generate-schema_stdout_1.snap | Updates generated schema snapshot description text. |
| scenario/snapshots/TestSnapshots_config_generate-schema_stdout_1.snap | Updates generated schema snapshot description text. |
| internal/projectconfig/overlay_file.go | Replaces loader-time expansion with ExpandResolvedOverlayFiles; changes no-match glob handling; validates overlays during overlay-file parsing. |
| internal/projectconfig/overlay_file_test.go | Updates and expands unit tests for new no-op semantics, per-overlay validation, and post-resolution expansion behavior. |
| internal/projectconfig/loader.go | Removes overlay-file expansion from raw config loading. |
| internal/projectconfig/component.go | Updates OverlayFiles doc/schema tag and changes merge semantics so non-empty overlay-files override inherited values. |
| internal/projectconfig/component_test.go | Adds tests covering overlay-files override vs inheritance behavior in merges. |
| internal/app/azldev/core/components/resolver.go | Expands overlay-files after config inheritance is applied, using an appropriate reference directory (config dir or spec dir). |
| internal/app/azldev/core/components/resolver_test.go | Adds resolver test ensuring spec-discovered components inherit and expand overlay-files defaults correctly. |
| docs/user/reference/config/overlays.md | Updates user docs for new inheritance behavior, reference-dir rules, and no-op no-match semantics. |
| docs/user/reference/config/components.md | Updates component config docs to reflect post-resolution expansion and inheritance semantics. |
Comment on lines
+463
to
+466
| // Apply inherited defaults to the component. | ||
| updatedComponentConfig, err = applyInheritedDefaultsToComponent(r.env, foundComponentConfig) | ||
| overlayFilesReferenceDir := overlayFilesReferenceDir(foundComponentConfig, specPath) | ||
|
|
||
| updatedComponentConfig, err = applyInheritedDefaultsToComponent(r.env, foundComponentConfig, overlayFilesReferenceDir) |
Resolve inherited overlay-file globs only after component defaults are merged so relative patterns can be interpreted for each concrete component. This also consumes the glob list after expansion to avoid duplicate overlay loading.
liunan-ms
force-pushed
the
liunan/overlay-files-post-resolution
branch
from
6924435 to
4b5f72c
Compare
Tonisal-byte
left a comment
Tonisal-byte
left a comment
Contributor
There was a problem hiding this comment.
Good work! Just one small comment
| return nil, fmt.Errorf("failed to scan for overlay files matching %q:\n%w", pattern, err) | ||
| case len(matches) == 0: | ||
| return nil, fmt.Errorf("%w: %q", ErrOverlayFilesNoMatches, pattern) | ||
| continue |
Contributor
There was a problem hiding this comment.
Should a no-match here really be silent? We dropped ErrOverlayFilesNoMatches, so a typo'd literal path (e.g. overlasy/foo.toml) now applies nothing with no error. Could we keep tolerating real globs but still error on literal paths that match nothing, like containsPattern does for includes in loader.go?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This updates
overlay-filesexpansion so per-file overlay documents are loaded after component config resolution instead of during raw config file loading.That means
overlay-filescan now be declared indefault-component-configat the project, distro, or component-group level and inherited by every applicable component. Relative patterns are interpreted for each concrete component, so a shared default such as:can discover component-local overlay files for each component.
Behavior Changes
Fixed #253