-
Notifications
You must be signed in to change notification settings - Fork 682
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
update supermarket profile search to use new type param #1289
Conversation
This should not merge until chef/supermarket#1463 is reviewed, merged, and released. Possibly the help output for |
I reckon that the failing Travis test for the functional test will not pass until chef/supermarket#1463 is released and running at supermarket.chef.io. @alexpop Care to review this and chef/supermarket#1463 ? |
chef/supermarket#1463 is merged and released to public Supermarket. 🎊 Force-pushed an amended-but-no-changes commit to trigger a re-run. |
09a5e3f
to
322e8c0
Compare
No longer WIP. The change is live on public Supermarket and the integration tests pass now that Travis got over today's hump. ✅ |
Awesome @robbkidd Could you please rebase your PR to latest master? |
url = "#{supermarket_url}/api/v1/tools" | ||
_success, data = get(url, { start: 0, items: 100, order: 'recently_added' }) | ||
url = "#{supermarket_url}/api/v1/tools-search" | ||
_success, data = get(url, { type: 'compliance_profile', items: 100, order: 'recently_added' }) |
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.
Just wondering if we really need the recently_added
order?
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.
Need? Maybe not. Depends on which makes sense as default: alphabetical by name
or reverse chronological by creation with recently_added
. Name might make more sense.
Sadly, v1 of the Supermarket API does not include next or previous references (a la HATEOAS) in the response, so seeing more than the first 100 of either set of results is left as an exercise to the client.
Reverts the work-around that pulls down the latest 100 tools and filters for type == 'compliance_profile' in the client. Go back to using tool-search with the new type parameter. Omit start:0 because that's the default. Keep the number of items returned at 100, which is more than the default 10. Signed-off-by: Robb Kidd <[email protected]>
322e8c0
to
ae474b8
Compare
Rebased! |
Thanks @robbkidd for this improvement! |
Reverts the work-around that pulls down the latest 100 tools
and filters for type == 'compliance_profile' in the client.
Go back to using tool-search with the new type parameter.
Omit start:0 because that's the default.
Keep the number of items returned at 100, which is more than the
default 10.
Closes #1255