New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
store, daemon, client, cmd/snap: expose "scope", default to wide #5479
Conversation
This adds support for the store's new `scope` query parameter, exposed directly to clients, and defaults the `snap find` to use `scope=wide` (i.e. get results for any architecture and branch unless it's `devmode`). Use `snap find --narrow <query>` to get back the old behaviour.
|
||
s.rsnaps = []*snap.Info{} | ||
|
||
req, err := http.NewRequest("GET", "/v2/find?q=foo&scope=creep", nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heh 👍
@@ -127,6 +127,7 @@ type FindOptions struct { | |||
Prefix bool | |||
Query string | |||
Section string | |||
Scope string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What can effectively be passed as scope here aside from wide
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
""
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good, couple of open points Qs
cmd/snap/cmd_find.go
Outdated
@@ -160,6 +161,7 @@ func init() { | |||
return &cmdFind{} | |||
}, map[string]string{ | |||
"private": i18n.G("Search private snaps"), | |||
"narrow": i18n.G("Only search for snaps in 'stable' and for the current architecture"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are we sure wide find other archs, it doesn't seem to be the case. I expect some potential bikeshedding on the name of this option
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sure we don't, now. I'll fix.
@@ -622,6 +622,7 @@ func searchStore(c *Command, r *http.Request, user *auth.UserState) Response { | |||
q := query.Get("q") | |||
section := query.Get("section") | |||
name := query.Get("name") | |||
scope := query.Get("scope") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we check whether scope is only "wide" or empty ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or from another POV, do we get a sane enough error if it's set to something else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see no harm in passing it through, as long as it's not exposed to the end-user directly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we get no error if it's set to something weird
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This adds support for the store's new
scope
query parameter, exposeddirectly to clients, and defaults the
snap find
to usescope=wide
(i.e. get results for any architecture and branch unless it's
devmode
).Use
snap find --narrow <query>
to get back the old behaviour.