feat(overlay-files): support {component} placeholder for pattern-discovered components#269
feat(overlay-files): support {component} placeholder for pattern-discovered components#269liunan-ms wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a {component} placeholder for project-level default-component-config.overlay-files to support pattern-based discovery of components and per-component overlay-file resolution, and updates schema/snapshots/docs accordingly.
Changes:
- Add projectconfig placeholder validation/splitting/substitution helpers and scope-based validation (
{component}required at project scope, forbidden elsewhere). - Add pattern-based component discovery from project-level
overlay-filesentries with resolver-level caching and deterministic unioning intoFindAllComponents. - Regenerate JSON schema / scenario snapshot and document the new placeholder behavior in the overlays config reference.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| schemas/azldev.schema.json | Updates schema description for overlay-files to document {component} rules. |
| scenario/snapshots/TestSnapshots_config_generate-schema_stdout_1.snap | Updates snapshot to match regenerated schema output. |
| internal/projectconfig/overlay_files_placeholder.go | Implements {component} placeholder validation, splitting, substitution, and absolutization helpers. |
| internal/projectconfig/overlay_files_placeholder_test.go | Unit tests for placeholder validation/splitting/substitution helpers. |
| internal/projectconfig/overlay_file.go | Substitutes {component} with component name before overlay glob expansion. |
| internal/projectconfig/configfile.go | Adds config-file validation enforcing placeholder rules by scope. |
| internal/projectconfig/configfile_test.go | Adds validation tests for allowed/forbidden placeholder usage across scopes. |
| internal/projectconfig/component.go | Updates OverlayFiles docs/schema description and absolutizes placeholder entries during WithAbsolutePaths. |
| internal/app/azldev/core/components/resolver.go | Unions pattern-discovered components into FindAllComponents with caching and shadowing behavior. |
| internal/app/azldev/core/components/pattern_discovery.go | Implements filesystem globbing + capture of {component} segment into synthesized components. |
| internal/app/azldev/core/components/pattern_discovery_test.go | Tests discovery, inheritance/override semantics, and same-scope collision errors. |
| docs/user/reference/config/overlays.md | Documents {component} placeholder discovery/inheritance/shadowing behavior for users. |
liunan-ms
force-pushed
the
liunan/overlayfiles-pattern
branch
from
4a56921 to
f2c8de7
Compare
| } | ||
|
|
||
| count := strings.Count(entry, OverlayFilesComponentPlaceholder) | ||
| if count == 0 { |
There was a problem hiding this comment.
I think this stops us having non-component placeholder entries, ie have overlay-files = ["static/overlays/*.toml"]. If this is expected, docs need to be updated (overlays.md:166) to match.
There was a problem hiding this comment.
This is intentional. Project-level entries without {component} are rejected because they would silently apply the same files to every component. Updated the doc.
| {name: "star before placeholder", entry: "comps*/{component}/*.overlay.toml", wantErr: true}, | ||
| {name: "whole segment at start", entry: "{component}/overlays/*.overlay.toml", wantErr: false}, | ||
| {name: "whole segment in middle", entry: "comps/{component}/overlays/*.overlay.toml", wantErr: false}, | ||
| {name: "whole segment at end", entry: "comps/overlays/{component}", wantErr: false}, |
There was a problem hiding this comment.
I think this might be an error, won't it glob files with the name of the component, not directories? I think it probably always needs to be afolder right?
There was a problem hiding this comment.
Great catch, updated the check and test.
| // declaration overrides pattern-discovered overlay files for the same name. | ||
| func logShadowedByDeclaration(shadowed *patternDiscoveredComponent) { | ||
| slog.Warn( | ||
| "project 'overlay-files' pattern match shadowed by explicit component declaration; overlay files will not be applied", |
There was a problem hiding this comment.
I think this might miss shadowing from project defaults by groups. Not sure if thats something worth an error though.
There was a problem hiding this comment.
This would be resolved by the scope restriction, now rejecting overlay-files in distro-version and component-group default-component-config entirely.
| // which suppresses inheritance (see [projectconfig.ComponentConfig.MergeUpdatesFrom]). | ||
| // A declaration with no overlay-files inherits the pattern and the | ||
| // discovered files still apply, so there's nothing to warn about. | ||
| if declared, ok := r.env.Config().Components[name]; ok && declared.OverlayFiles != nil { |
There was a problem hiding this comment.
Might want to use len(declared.OverlayFiles) > 0 rather than nil, if I recall from the previous PR the way to disable inheritance is to set overlay-files=[]. 50/50 on this, we could still warn because thats weird, but it is intentionally explicitly saying "no overlays" for a component.
There was a problem hiding this comment.
Fixed. Changed to len(declared.OverlayFiles) > 0.
| } | ||
|
|
||
| prefix := entry[:idx] | ||
| if globMetaIdx := strings.IndexAny(prefix, "*?["); globMetaIdx >= 0 { |
There was a problem hiding this comment.
Might be able to add https://pkg.go.dev/github.com/bmatcuk/doublestar/v4#ValidatePattern here as well, get early signal back on a bad glob pattern.
There was a problem hiding this comment.
Now using doublestar.ValidatePattern on the substituted glob validation.
| err error | ||
| } | ||
|
|
||
| func (c *discoveredComponentsCache) load(env *azldev.Env) (map[string]*patternDiscoveredComponent, error) { |
There was a problem hiding this comment.
Found this a while back, might do the trick here: https://pkg.go.dev/sync#OnceValues
There was a problem hiding this comment.
switched to using sync.OnceValues
| } | ||
|
|
||
| byName := make(map[string]*patternDiscoveredComponent) | ||
| // Deduplicate files per component before appending to preserve stable ordering. |
There was a problem hiding this comment.
Curious if this is needed, my understanding was that glob would deduplicate internally.
There was a problem hiding this comment.
doublestar.Glob returns unique matches per call so the seen map was dead code and has been removed.
liunan-ms
force-pushed
the
liunan/overlayfiles-pattern
branch
from
f2c8de7 to
1f37e09
Compare
liunan-ms
force-pushed
the
liunan/overlayfiles-pattern
branch
from
1f37e09 to
27e7ffb
Compare
liunan-ms
force-pushed
the
liunan/overlayfiles-pattern
branch
from
27e7ffb to
594c51b
Compare
|
Thanks @dmcilvaney for the review! After thinking about the scope and usage of
I update the implementation and now the scope rule is enforced.
|
liunan-ms
force-pushed
the
liunan/overlayfiles-pattern
branch
from
594c51b to
fc2fd2b
Compare
…overed components
Add '{component}' placeholder in project-level overlay-files entries. The resolver globs matching paths, extracts each captured segment as a component name, and synthesizes components not otherwise declared. Explicit component declarations shadow pattern-discovered matches (warning emitted).
- projectconfig: validate placeholder syntax per scope; reject backslash separators and glob metachars before the placeholder
- components: pattern_discovery walks overlay-files entries; Resolver caches discovery results per instance
- schema/docs: user-facing overlays.md updated
liunan-ms
force-pushed
the
liunan/overlayfiles-pattern
branch
from
fc2fd2b to
0573473
Compare
| func absolutizeOverlayFilesPlaceholderEntries(referenceDir string, entries []string) []string { | ||
| if entries == nil { | ||
| return nil | ||
| } | ||
|
|
||
| result := make([]string, len(entries)) |
| // - Distro-version and component-group 'default-component-config': | ||
| // 'overlay-files' is not allowed at all. Broad-scope defaults for this | ||
| // field silently displace project-level discovery and per-component | ||
| // overrides, so we require callers to place the list at project or | ||
| // component scope where the intent is unambiguous. |
Summary
Adds a
{component}placeholder for project-leveloverlay-filesentries. The resolver globs matching paths, extracts each captured segment as a component name, and synthesizes components not otherwise declared. Explicit component declarations shadow pattern-discovered matches.Details
\as a path separator and glob metachars (*?[) before the placeholder (permissive in the suffix).pattern_discovery.gowalks project-leveloverlay-filesentries containing{component}, resolves matching filesystem paths via doublestar, and captures the component name from the placeholder position.Resolvercaches discovery results per-instance so repeated calls don't re-glob.FindAllComponentsunions the three sources — group members, loose declared components, pattern-discovered components — and emits sorted output for determinism.docs/user/reference/config/overlays.mddocuments the placeholder.