settings: add ntp settings extension #3595
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sam-berning
force-pushed
the
extract-ntp-settings-model
branch
from
a53a45d to
751b7d4
Compare
cbgbt
reviewed
Nov 15, 2023
| model-derive = { path = "../../models/model-derive", version = "0.1" } | ||
| serde = { version = "1", features = ["derive"] } | ||
| serde_json = "1" | ||
| string_impls_for = { version = "0.1", path = "../../models/string_impls_for" } |
There was a problem hiding this comment.
Looks like we don't use this dependency, you can remove it.
| edition = "2021" | ||
| publish = false | ||
|
|
||
| [dependencies] |
There was a problem hiding this comment.
The settings-sdk emits logs to stderr, and I realized after talking with @zmrow that I never initialized the logger in the motd extension.
I would take a dependency on env_logger and initialize the logger just before initializing the extension with env_logger::init().
Comment on lines
41
to
42
| // TODO: add additional validation -- legal for this to be set to any URL or do we need to | ||
| // check that it is a valid time server? |
There was a problem hiding this comment.
We tend not to rely on external services when validating settings as long as they are the same shape, and I think that's the right move. It would be too easy for a transient error to cause unnecessary failure at scale.
sam-berning
force-pushed
the
extract-ntp-settings-model
branch
from
751b7d4 to
3c28977
Compare
Signed-off-by: Sam Berning <[email protected]>
Signed-off-by: Sam Berning <[email protected]>
Signed-off-by: Sam Berning <[email protected]>
sam-berning
force-pushed
the
extract-ntp-settings-model
branch
from
3c28977 to
02d4ce1
Compare
cbgbt
approved these changes
Nov 16, 2023
zmrow
approved these changes
Nov 16, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of changes:
Implements a settings extension for NTP settings using the
bottlerocket-settings-sdk.Because it uses the SDK, this will create a settings binary which can be installed on a variant via the
settings-ntppackage. But for now, we are only using the settings model from the extension.Testing done:
aws-devon an ec2 instance and checked that the NTP settings were set correctly on boot and would update/etc/chrony.confwhen set to a new valueaws-dev, and made sure the binary gave the expected responses.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.