-
Notifications
You must be signed in to change notification settings - Fork 524
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
apiclient: add 'apply' mode for applying settings from URIs #1391
Conversation
Having types named "check", "apply", and "cancel" that are related to updates, but don't have an "update" prefix, prevents us from having other types or subcommands called "check", "apply", or "cancel". This adds an "update" prefix to each of them to make it clear what they're checking, applying, or canceling.
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.
Nice work. Just a quick question.
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.
🪀
f3644e2
to
40c6713
Compare
^ This push addresses @webern's good catch about error_for_status. (It wouldn't have applied before (unless an HTTP server gave a TOML settings representation in their error response) but the error would have been confusing.) Before:
After:
Repeated all prior testing as well. |
/// Generates a random ID, affectionately known as a 'rando'. | ||
pub(crate) fn rando() -> String { | ||
thread_rng() | ||
.sample_iter(&Alphanumeric) | ||
.take(16) | ||
.map(char::from) | ||
.collect() | ||
} |
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 split this into its own crate? We've got a copy in a few places:
- models build script
- storewolf
- migrator
|
40c6713
to
4004520
Compare
^ This push makes three changes:
One expected use of JSON/stdin is for things like https://github.com/mozilla/sops, which could help with encrypting some data; it doesn't support TOML, hence JSON, and you wouldn't want to store the file on disk after decryption, hence stdin rather than requiring creation of a file. But they're generally useful, too. I repeated all testing in the description above, and I'm adding some more test logs there now. |
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.
Nice work with the future
s!
🏈
Description of changes:
There are a few potential benefits here:
file://settings.toml
URIs open the way to potentially store settings in Bottlerocket images, rather than compiled into storewolf, for cases where having them available at runtime would be handy. (Potentially as default backfill for deleted settings, or explicit requests to "return to default". Potentially as alternate sets of defaults..?)apiclient apply
could be called from a host container or bootstrap container. We could have a default bootstrap container that applies a conveniently listed set of URLs, or something - just a thought. (The downside there is that you're taking on some level of risk from potential network failure and inability to apply settings.) Or settings files could be baked into custom host/bootstrap containers.One potential use of JSON/stdin mode is https://github.com/mozilla/sops, which could help with encrypting inputs like keys; it doesn't support TOML, hence JSON, and you wouldn't want to store the file on disk after decryption, hence stdin rather than requiring creation of a file. But they're generally useful, too.
Testing done:
Happy tests:
apiclient apply 'https://bla/settings-1.toml' 'https://bla/settings-2.toml'
, and saw settings from both apply to the system.file://
URL and a file I created in /tmp.Failure tests, to ensure errors are clear (CLICK ME)
[settings]
block:In each failure scenario, as expected, the settings from 'good' URLs were not applied. We want all-or-nothing behavior in a single apiclient call.
stdin/JSON tests (CLICK ME)
Input on stdin, ending with ctrl-d:
Input piped in:
Input typed in, in between some URIs, where good-url-1 sets a motd and good-url-2 sets lockdown:
As before, no settings are applied if any settings fail:
The last input given takes effect, even if the earlier one takes (much) longer to retrieve, because I typed it by hand:
The same thing works with JSON:
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.