settings: add host-containers settings extension #3772
Conversation
| async fn get_host_containers<P>(socket_path: P) -> Result<HashMap<Identifier, model::HostContainer>> | ||
| async fn get_host_containers<P>( | ||
| socket_path: P, | ||
| ) -> Result<HashMap<Identifier, settings_extension_host_containers::HostContainer>> | ||
| where | ||
| P: AsRef<Path>, | ||
| { |
There was a problem hiding this comment.
Below this we are calling API Server. In other other PRs, e.g. corndog, prariedog, et. al., we are eliminating reliance on the API Server by changing to a config. We will be doing that here as well right? Are we deciding to make such changes in two steps instead of one PR?
If we do, it raises the question of whether the config should be modeled as a map of settings_extension_host_containers::HostContainer structures. In other cases I don't think we have re-used the settings extension model directly in the config file.
There was a problem hiding this comment.
I was imagining that the setting extension and the config file change would be two PRs, I think they should be relatively separate changes. I can take the config file work and get a PR out for that first.
If we do, it raises the question of whether the config should be modeled as a map of
settings_extension_host_containers::HostContainerstructures. In other cases I don't think we have re-used the settings extension model directly in the config file.
The dogs should define their own structs to model the config files. We shouldn't depend on the settings model structs because those are no longer the source of truth for the setting shape, and we shouldn't depend on settings extensions because we can't guarantee that they'll be there
There was a problem hiding this comment.
2 PRs seems fine. If I understand correctly (and this is how I would want it): host-containers will no longer depend on the settings-extension once it has a config file, right?
There was a problem hiding this comment.
yep, that's what I was thinking
Signed-off-by: Sam Berning <bernings@amazon.com>
Signed-off-by: Sam Berning <bernings@amazon.com>
Because host-containers still reads settings from the API, it reads them to the variant's settings model, which means that now it needs to pull in the host-containers settings extension model. This change is only needed until host-containers is migrated to no longer get settings directly from the API, and instead gets them from a config file. Signed-off-by: Sam Berning <bernings@amazon.com>
Signed-off-by: Sam Berning <bernings@amazon.com>
sam-berning
force-pushed
the
host-container-settings-extension
branch
from
d6af075
to
e5b3592
Compare
|
^ fixed fmt errors |
| impl Serialize for HostContainersSettingsV1 { | ||
| fn serialize<S>(&self, serializer: S) -> std::result::Result<S::Ok, S::Error> | ||
| where | ||
| S: Serializer, | ||
| { | ||
| self.host_containers.serialize(serializer) | ||
| } | ||
| } | ||
|
|
||
| impl<'de> Deserialize<'de> for HostContainersSettingsV1 { | ||
| fn deserialize<D>(deserializer: D) -> std::result::Result<Self, D::Error> | ||
| where | ||
| D: Deserializer<'de>, | ||
| { | ||
| let host_containers = HashMap::deserialize(deserializer)?; | ||
| Ok(Self { host_containers }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Are these necessary? Can you derive Serialize and Deserialize?
| } | ||
| } | ||
|
|
||
| #[model(impl_default = true)] |
There was a problem hiding this comment.
Same question I had before. What are we using the model macro for? Can we do whatever it's doing manually instead. I think all it is doing is making these Options and implementing Serialize and Deserialize. This would be better for readability if it were done manually.
Issue number:
Closes #3650
Description of changes:
Creates a host-containers settings extension.
Because host-containers still depends on the API to get settings, there are some changes in here to use the settings extension model within
host-containers. This is definitely not best practice in the long run, but it should be stable for now and will be removed once we migratehost-containersto read settings from a config file instead. We can discuss whether we should wait to merge this PR until that change is in.Testing done:
Built an
aws-devAMI and ran an instance with it. I tested both thathost-containersworked as expected (considering I got into the instance via the admin container, it worked just fine) and that the settings extension gave responses 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.