-
Notifications
You must be signed in to change notification settings - Fork 508
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
settings: add network settings extension #3712
settings: add network settings extension #3712
Conversation
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.
This looks right to me, other than the need to express a dependency on netdog in the RPM. Since this requires #3700, we should either open an issue or merge this after that PR has landed.
) -> Result<GenerateResult<Self::PartialKind, Self>> { | ||
let partial = existing_partial.unwrap_or_default(); | ||
|
||
// OpenQ: how to make this extension depend on netdog? |
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.
This should be expressed as an RPM dependency.
glibc = { path = "../glibc" } | ||
|
||
# RPM Requires | ||
[dependencies] |
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.
As mentioned elsewhere, we'll want to add the netdog RPM as a dependency here.
License: Apache-2.0 OR MIT | ||
URL: https://github.com/bottlerocket-os/bottlerocket | ||
|
||
BuildRequires: %{_cross_os}glibc-devel |
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.
Likewise, we want to add a Requires: %{_cross_os}netdog
This symbol would only be available once we merge in #3700
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.
Likewise, we want to add a Requires: %{_cross_os}netdog
This symbol would only be available once we merge in #3700
Isn't it the other way around? Doesn't netdog
depend on its settings extension?
Edit: oh I see, this extension calls out to netdog
makes sense. This is a setting that relies on netdog to find its value.
License: Apache-2.0 OR MIT | ||
URL: https://github.com/bottlerocket-os/bottlerocket | ||
|
||
BuildRequires: %{_cross_os}glibc-devel |
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.
Likewise, we want to add a Requires: %{_cross_os}netdog
This symbol would only be available once we merge in #3700
Isn't it the other way around? Doesn't netdog
depend on its settings extension?
Edit: oh I see, this extension calls out to netdog
makes sense. This is a setting that relies on netdog to find its value.
sources/models/Cargo.toml
Outdated
settings-extension-ntp = { path = "../settings-extensions/ntp", version = "0.1" } | ||
settings-extension-container-registry = { path = "../settings-extensions/container-registry", version = "0.1" } | ||
settings-extension-updates = { path = "../settings-extensions/updates", version = "0.1" } | ||
settings-extension-network = { path = "../settings-extensions/network", version = "0.1" } |
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.
settings-extension-ntp = { path = "../settings-extensions/ntp", version = "0.1" } | |
settings-extension-container-registry = { path = "../settings-extensions/container-registry", version = "0.1" } | |
settings-extension-updates = { path = "../settings-extensions/updates", version = "0.1" } | |
settings-extension-network = { path = "../settings-extensions/network", version = "0.1" } | |
settings-extension-container-registry = { path = "../settings-extensions/container-registry", version = "0.1" } | |
settings-extension-network = { path = "../settings-extensions/network", version = "0.1" } | |
settings-extension-ntp = { path = "../settings-extensions/ntp", version = "0.1" } | |
settings-extension-updates = { path = "../settings-extensions/updates", version = "0.1" } |
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.
Maybe keep these sorted?
}) | ||
} else { | ||
let hostname = parse_stdout(String::from_utf8_lossy(&ret.stdout).to_string()); | ||
Ok(ValidLinuxHostname::try_from(hostname).context(InvalidHostnameSnafu {})?) |
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.
Can the braces be eliminated in these zero params Snafu struct constructions? (can't remember)
Ok(ValidLinuxHostname::try_from(hostname).context(InvalidHostnameSnafu {})?) | |
Ok(ValidLinuxHostname::try_from(hostname).context(InvalidHostnameSnafu)?) |
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.
looks like they can! updating
} | ||
|
||
fn validate(_value: Self, _validated_settings: Option<serde_json::Value>) -> Result<()> { | ||
Ok(()) |
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.
Ok(()) | |
// This settings field types handle validation during deserialization. | |
Ok(()) |
|
||
#[test] | ||
fn test_serde_network() { | ||
let test_json = r#"{ |
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.
Add a test where the JSON fails to parse due to an invalid hostname or some other invalid field. It helps document the way the type enforces correctness of its input.
Signed-off-by: Sam Berning <bernings@amazon.com>
Signed-off-by: Sam Berning <bernings@amazon.com>
Signed-off-by: Sam Berning <bernings@amazon.com>
02f259e
to
f2d95fc
Compare
Issue number:
Closes #3652
Description of changes:
Creates a network settings extension
Testing done:
Unit testing, built
aws-dev
variant and packagedsettings-network
in it. Tested on an ec2 instance.network settings worked as expected, and the setting extension generation returned as expected:
(EDIT 1-18)
Also built and tested a
vmware-dev
image, and the extension worked as expected: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.