feat(store): add StoreMux wildcard scope fallback#2560
Draft
fseldow wants to merge 1 commit into
Draft
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR updates store and executor scope handling to introduce a * “fallback” concept, allowing a fallback store/executor to be registered and used when no more-specific scope matches.
Changes:
- Treat store scope
*as a fallback store registration instead of an invalid scope. - Add fallback executor registration/matching when scope is
*. - Update/extend unit tests to cover fallback behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| internal/store/factory.go | Implements fallback-store registration semantics and adjusts scope handling logic. |
| internal/store/factory_test.go | Updates existing cases and adds coverage for fallback-store scenarios. |
| internal/executor/executor.go | Adds fallback executor registration and matching behavior for *. |
| internal/executor/executor_test.go | Updates tests to validate fallback executor registration/matching and error behavior. |
Comments suppressed due to low confidence (1)
internal/store/factory.go:108
- The fallback scope
*is only treated specially when it is the only scope. If a configuration includes*alongside other scopes (e.g.,[]string{"*", "example.com"}), the loop will attemptRegister("*", store), which likely reintroduces the “invalid scope” error path or ambiguous semantics. Consider explicitly rejecting any scope list that contains*plus other entries (with a clear error), or handle*as fallback whenever it appears and require it to be exclusive.
for _, scope := range scopes {
if err = storeMux.Register(scope, store); err != nil {
return nil, fmt.Errorf("failed to register store for scope %q: %w", scope, err)
}
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+83
to
89
| scopes := storeOptions.Scopes | ||
| if len(scopes) == 0 { | ||
| // if no scopes are provided, use the global scopes of the executor. | ||
| storeOptions.Scopes = globalScopes | ||
| } | ||
| if len(storeOptions.Scopes) == 0 { | ||
| return nil, fmt.Errorf("store options must contain at least one scope") | ||
| scopes = globalScopes | ||
| } | ||
|
|
||
| store, err := newStore(storeOptions) |
Comment on lines
+94
to
+99
| if len(scopes) == 1 && scopes[0] == "*" { | ||
| if err = storeMux.RegisterFallback(store); err != nil { | ||
| return nil, fmt.Errorf("failed to register fallback store: %w", err) | ||
| } | ||
| continue | ||
| } |
Comment on lines
+63
to
+73
| if len(opts) == 1 && len(opts[0].Scopes) == 0 && len(globalScopes) == 0 { | ||
| store, err := newStore(opts[0]) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to create store for type %q: %w", opts[0].Type, err) | ||
| } | ||
| storeMux := ratify.NewStoreMux() | ||
| if err = storeMux.RegisterFallback(store); err != nil { | ||
| return nil, fmt.Errorf("failed to register fallback store: %w", err) | ||
| } | ||
| return storeMux, nil | ||
| } |
Comment on lines
+83
to
87
| scopes := storeOptions.Scopes | ||
| if len(scopes) == 0 { | ||
| // if no scopes are provided, use the global scopes of the executor. | ||
| storeOptions.Scopes = globalScopes | ||
| } | ||
| if len(storeOptions.Scopes) == 0 { | ||
| return nil, fmt.Errorf("store options must contain at least one scope") | ||
| scopes = globalScopes | ||
| } |
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (68.00%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #2560 +/- ##
==========================================
+ Coverage 77.62% 77.65% +0.03%
==========================================
Files 105 105
Lines 4657 4677 +20
==========================================
+ Hits 3615 3632 +17
+ Misses 893 889 -4
- Partials 149 156 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: xinhl <xinhl@microsoft.com>
fseldow
force-pushed
the
xinhl/alpha2-storemux-fallback
branch
from
2405092 to
087f0d0
Compare
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
*as the fallback scope when wiring StoreMux storesTesting