Skip to content
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

API: support prefix query parameter consistently #1835

Merged
merged 2 commits into from Nov 30, 2021

Conversation

tjkirch
Copy link
Contributor

@tjkirch tjkirch commented Nov 19, 2021

Description of changes:

40cacee0 API: support prefix query parameter consistently
57074659 API: add missing OpenAPI definition for /

apiclient get (#1836) supports prefix queries of the API, and this PR is needed to consistently support the ?prefix= parameter for all of the different data types - settings (which was already supported), services, configuration files, and os. (apiclient get just queries / but consistency is good.)

Testing done:

Existing and new unit tests pass.

Started a 1.4.1 AMI and a new one with these changes.

The following queries return exactly the same response (after keys are sorted) modulo the build version, IP address, and update seed, as expected:

  • apiclient -u /
  • apiclient -u /settings
  • apiclient -u /settings?prefix=m shows motd, metrics, etc.
  • apiclient -u /settings?prefix=motd shows motd
  • apiclient -u /settings?prefix=x shows nothing
  • apiclient -u /services
  • apiclient -u /configuration-files
  • apiclient -u /os

The following queries used to ignore the unknown prefix query and return all results, but now properly restrict to the given prefix:

  • apiclient -u /?prefix=x shows nothing
  • apiclient -u /?prefix=s shows settings and services
  • apiclient -u /services?prefix=x shows nothing
  • apiclient -u /services?prefix=m shows motd and metricdog
  • apiclient -u /configuration-files?prefix=x shows nothing
  • apiclient -u /configuration-files?prefix=m shows motd and metricdog-toml
  • apiclient -u /os?prefix=x shows nothing
  • apiclient -u /os?prefix=a shows arch

Terms of contribution:

By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.

@bcressey
Copy link
Contributor

apiclient -u /settings?prefix=m shows motd, metrics, etc.

Interesting - I've only used prefix as a destructuring operation to get at the subset of interesting data, and just assumed it was implemented in terms of full components rather than string prefixes.

It's worth considering whether that'd be more useful than the string prefix version. Obviously it makes it kind of useless for the flat structures returned by /os, but on the other hand it's hard to see much of a use case for returning the data from both motd and metricdog, and if someone were relying on that, it'd be frustrating to only be able to compose shapes with matching prefixes.

If it were implemented in terms of full components, we could allow multiple prefixes to be specified, e.g.:
apiclient -u /settings?prefix=motd;prefix=metricdog

Copy link
Contributor

@zmrow zmrow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM code-wise, and I'm not opposed to shipping as-is.

@bcressey 's comment is something to consider though. 🤔

@zmrow
Copy link
Contributor

zmrow commented Nov 22, 2021

apiclient -u /settings?prefix=motd;prefix=metricdog seems like a nice way to query multiple things rather than prefix=m, but I also don't think it's unreasonable to keep it simple and ask for multiple queries.

@tjkirch tjkirch merged commit cb745ae into bottlerocket-os:develop Nov 30, 2021
@tjkirch tjkirch deleted the api-prefix branch November 30, 2021 18:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants