Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Add new store endpoints & snap find support for sections handling #2288
Conversation
and others
added some commits
Oct 27, 2016
All checks have failed
Some checks were not successful
Some checks were not successful
| @@ -885,6 +903,8 @@ func (s *Store) Find(search *Search, user *auth.UserState) ([]*snap.Info, error) | ||
| } else { | ||
| q.Set("q", searchTerm) | ||
| } | ||
| + fmt.Println(search.Section) |
AlexandreAbreu
added some commits
Nov 17, 2016
All checks have passed
All checks have passed
AlexandreAbreu
changed the title from
[WIP] Add new store endpoints & snap find support for sections handling
to
Add new store endpoints & snap find support for sections handling
Nov 18, 2016
chipaca
requested changes
Nov 21, 2016
This looks very good! I especially liked that you've implemented a Completer for it.
I'm marking it as "request changes" because of the retry dance; all the rest are trivials.
Good job!
| @@ -84,8 +84,22 @@ func getPriceString(prices map[string]float64, suggestedCurrency, status string) | ||
| return formatPrice(price, currency) | ||
| } | ||
| +type SectionName string |
| @@ -96,20 +110,17 @@ func init() { | ||
| return &cmdFind{} | ||
| }, map[string]string{ | ||
| "private": i18n.G("Search private snaps"), | ||
| + "section": i18n.G("Restrict the search to a given section name"), |
| + case store.ErrEmptyQuery, store.ErrBadQuery: | ||
| + return BadRequest("%v", err) | ||
| + case store.ErrUnauthenticated: | ||
| + return Unauthorized(err.Error()) |
chipaca
Nov 21, 2016
Member
why ("%v", err) in the others and err.Error() here? AFAIK they're both the same unless err==nil, which you already split out explicitly
| + return InternalError("%v", err) | ||
| + } | ||
| + | ||
| + results := make([]*json.RawMessage, 0, len(sections)) |
chipaca
Nov 21, 2016
Member
I'm missing something here. sections is a []string, so why not just return it? As in return SyncResponse(sections, meta)?
| + URL: &u, | ||
| + Accept: halJsonContentType, | ||
| + } | ||
| + resp, err := s.doRequest(s.client, reqOptions, user) |
chipaca
Nov 21, 2016
Member
as this is new code, could you write it already doing the retry dance? Search for retry.Start in this same file for examples. We're slowly rewriting all things that call doRequest to work this way, but we shouldn't add to the backlog.
| + } | ||
| + defer resp.Body.Close() | ||
| + if resp.StatusCode != 200 { | ||
| + return nil, respToError(resp, "search") |
| + } | ||
| + | ||
| + if ct := resp.Header.Get("Content-Type"); ct != halJsonContentType { | ||
| + return nil, fmt.Errorf("received an unexpected content type (%q) when trying to search via %q", ct, resp.Request.URL) |
| + | ||
| + dec := json.NewDecoder(resp.Body) | ||
| + if err := dec.Decode(§ionData); err != nil { | ||
| + return nil, fmt.Errorf("cannot decode reply (got %v) when trying to get sections via %q", err, resp.Request.URL) |
mvo5
reviewed
Nov 21, 2016
Looks good, thank you. One question - it looks like now we do allow a snap find without further arguments again. AIUI this will mean that we get the first 100 snaps again as "featured". But right now AFAICT the store does not support this so we get a random list of 100 snaps (which may include useless ones like test-snapd-python-webserver). If that is indeed the case, should we keep the restriction for snap find for now (until the store has support for featured)?
| +// GetSections returns the list of existing snap sections in the store | ||
| +func (client *Client) GetSections() ([]string, error) { | ||
| + var sections []string | ||
| + _, err := client.doSync("GET", "/v2/sections", nil, nil, nil, §ions) |
mvo5
Nov 21, 2016
Collaborator
(nitpick) This and the following line could be combined into a single: if _err := client.doSync(..); err != nil { line. But its not really important (just an idiom we use a lot in the code).
AlexandreAbreu
Nov 21, 2016
Contributor
Well maybe but I did it this way to stay consistent with the rest of the code that does not seem to merge assignment/tests,
| @@ -84,8 +84,22 @@ func getPriceString(prices map[string]float64, suggestedCurrency, status string) | ||
| return formatPrice(price, currency) | ||
| } | ||
| +type SectionName string | ||
| + | ||
| +func (s *SectionName) Complete(match string) []flags.Completion { |
| @@ -153,7 +153,6 @@ The snap tool interacts with the snapd daemon to control the snappy software pla | ||
| cmd, err := parser.AddCommand(c.name, c.shortHelp, strings.TrimSpace(c.longHelp), obj) | ||
| if err != nil { | ||
| - |
AlexandreAbreu
added some commits
Nov 21, 2016
All checks have passed
Some checks were not successful
All checks have passed
niemeyer
requested changes
Nov 21, 2016
Looks pretty good, thanks for contributing it.
A few comments:
| @@ -129,6 +130,16 @@ func (client *Client) List(names []string) ([]*Snap, error) { | ||
| return result, nil | ||
| } | ||
| +// GetSections returns the list of existing snap sections in the store | ||
| +func (client *Client) GetSections() ([]string, error) { |
mvo5
Nov 22, 2016
Collaborator
(also maybe interessting in this context in "effective go" which is a interesting read: https://golang.org/doc/effective_go.html#Getters)
| @@ -150,6 +161,7 @@ func (client *Client) Find(opts *FindOptions) ([]*Snap, *ResultInfo, error) { | ||
| case opts.Private: | ||
| q.Set("select", "private") | ||
| } | ||
| + q.Set("section", opts.Section) |
niemeyer
Nov 21, 2016
Contributor
if opts.Section != "" { ... }, so we continue to not send it unless requested? Feels slightly cleaner than sending an empty string which is ignored.
| @@ -43,7 +43,7 @@ func (cs *clientSuite) TestClientFindRefreshSetsQuery(c *check.C) { | ||
| c.Check(cs.req.Method, check.Equals, "GET") | ||
| c.Check(cs.req.URL.Path, check.Equals, "/v2/find") | ||
| c.Check(cs.req.URL.Query(), check.DeepEquals, url.Values{ | ||
| - "q": []string{""}, "select": []string{"refresh"}, | ||
| + "q": []string{""}, "section": []string{""}, "select": []string{"refresh"}, |
| +type SectionName string | ||
| + | ||
| +func (s *SectionName) Complete(match string) []flags.Completion { | ||
| + // TODO cache the result |
niemeyer
Nov 21, 2016
Contributor
Should be cached on the snapd daemon end rather than here, which means we can cache in memory instead of on disk. So it's okay to drop the TODO here.
| +func (s *SectionName) Complete(match string) []flags.Completion { | ||
| + // TODO cache the result | ||
| + cli := Client() | ||
| + sections, _ := cli.GetSections() |
niemeyer
Nov 21, 2016
Contributor
if err != nil { return nil }, more explicit and clear than just ignoring the error silently, which tends to raise eyebrows. Also saves the allocation bellow, but that's an irrelevant saving in this context.
| type cmdFind struct { | ||
| - Private bool `long:"private"` | ||
| + Private bool `long:"private"` | ||
| + Section SectionName `long:"section" optional:"yes"` |
niemeyer
Nov 21, 2016
Contributor
"optional" is the default, isn't it? Otherwise "private" above would be bogus. Can we please drop the explicit listing if so?
| }, []argDesc{{name: i18n.G("<query>")}}) | ||
| } | ||
| func (x *cmdFind) Execute(args []string) error { | ||
| if len(args) > 0 { | ||
| return ErrExtraArgs | ||
| } | ||
| - | ||
| - if x.Positional.Query == "" { |
| s.RedirectClientToTestServer(func(w http.ResponseWriter, r *http.Request) { | ||
| - switch n { | ||
| - case 0: | ||
| + if n < maxfindRequest { |
niemeyer
Nov 21, 2016
Contributor
The changes in this test function don't feel entirely sound. The changes generalized the logic, making it less readable, and didn't really add anything meaningful that would require that generalization. By the end of the day in both cases it simply sends a string that goes through the same code path.
If the goal is checking that the empty string does not fail, then let's simply invert the test that got removed (TestFindNothingFails => TestFindNothingWorks) instead of fiddling with this test function.
| + } | ||
| + | ||
| + meta := &Meta{ | ||
| + Sources: []string{"store"}, |
niemeyer
Nov 21, 2016
Contributor
Sources is a bit of historical baggage from the ancient API which mixed store and local snaps under a single endpoint. I hope we can even drop that entirely at some point, so it's probably worth keeping this out of this new endpoint.
| +type sectionResults struct { | ||
| + Payload struct { | ||
| + Sections []struct{ Name string } `json:"clickindex:sections"` | ||
| + } `json:"_embedded"` |
niemeyer
Nov 21, 2016
Contributor
I generally don't complain much about store APIs since they are invisible to the end user, but is there much benefit in holding up to those unfriendly names?
| @@ -914,6 +932,7 @@ func (s *Store) Find(search *Search, user *auth.UserState) ([]*snap.Info, error) | ||
| } else { | ||
| q.Set("q", searchTerm) | ||
| } | ||
| + q.Set("section", search.Section) |
niemeyer
Nov 21, 2016
Contributor
Same as client end, seems a bit cleaner to use this only when non-empty.
mvo5
Nov 23, 2016
Collaborator
Its "funny". This is actually required for the store to work currently. It behaves differently for section= than for a missing section in the query:
$ curl -s -H "accept: application/hal+json" -H "X-Ubuntu-Release: 16" -H "X-Ubuntu-Device-Channel: stable" -H "X-Ubuntu-Wire-Protocol: 1" -H "X-Ubuntu-Architecture: amd64" 'https://search.apps.ubuntu.com/api/v1/snaps/search?section=' |python3 -m json.tool|grep '"title"'
"title": "Docker",
"title": "lxd",
"title": "mongo32",
"title": "Rocket Chat Server",
However:
$ curl -s -H "accept: application/hal+json" -H "X-Ubuntu-Release: 16" -H "X-Ubuntu-Device-Channel: stable" -H "X-Ubuntu-Wire-Protocol: 1" -H "X-Ubuntu-Architecture: amd64" 'https://search.apps.ubuntu.com/api/v1/snaps/search?q=' |python3 -m json.tool|grep '"title"'|wc -l
100
i.e. the later query will just return everything (paginated).
| + | ||
| + q := u.Query() | ||
| + | ||
| + q.Set("confinement", "strict") |
niemeyer
Nov 21, 2016
Contributor
Why is this necessary? We are asking for store sections.. sections don't have confinement settings? Doesn't feel right.
| + | ||
| + return sectionNames, nil | ||
| + } | ||
| + panic("unreachable") |
| +*/ | ||
| +const MockSectionsJSON = `{ | ||
| + "_embedded": { | ||
| + "clickindex:sections": [ |
niemeyer
Nov 21, 2016
Contributor
There's probably not much gain in having 20 lines here instead of:
"clickindex:sections": [{"name": "featured"}, {"name": "database"}],
| + repo := New(&cfg, nil) | ||
| + c.Assert(repo, NotNil) | ||
| + | ||
| + _, err := repo.Sections(t.user) |
| + expected="(?s)Name +Version +Developer +Notes +Summary *\n\ | ||
| + (.*?\n)?\ | ||
| + .*" | ||
| + snap find | grep -Pzq "$expected" |
niemeyer
Nov 21, 2016
Contributor
Can we test something we know will tend to remain featured here, so we have a hint that the feature in fact works?
AlexandreAbreu
and others
added some commits
Nov 22, 2016
All checks have passed
|
@niemeyer @AlexandreAbreu I would love to get this into 2.18, this is why I picked it up in #2333 and did some minor tweaks (most importantly to use the new store.retryRequest() helper and minor tweaks to respond to Gustavos feedback). Feel free to cherry-pick in here if its not reviewed/merged. If it misses 2.18 thats fine, I think we just continue in here in this case. One thing (maybe not for this branch) I noticed is that If we translated that to a go error we could convert it to an error-kind in our REST api and the client could show a nicer error message. But probably a branch on its own :) |
mvo5
and others
added some commits
Nov 23, 2016
All checks have passed
All checks have passed
mvo5
added this to the
2.18 milestone
Nov 24, 2016
|
@AlexandreAbreu Yeah, the hardcoding of that string is not ideal, I think we want to tweak that in the future. |
Some checks were not successful
| - return errors.New(i18n.G("you need to specify a query. Try \"snap find hello-world\".")) | ||
| + // magic! `snap find` returns the featured snaps | ||
| + if x.Positional.Query == "" && x.Section == "" { | ||
| + x.Section = "featured" |
| +*/ | ||
| +const MockSectionsJSON = `{ | ||
| + "_embedded": { | ||
| + "clickindex:sections": [ |
niemeyer
Nov 21, 2016
Contributor
There's probably not much gain in having 20 lines here instead of:
"clickindex:sections": [{"name": "featured"}, {"name": "database"}],
AlexandreAbreu commentedNov 17, 2016
•
Edited 1 time
-
AlexandreAbreu
Nov 18, 2016
Add new store endpoints & snap find support for sections handling.
The sections endpoint is currently in the staging store.