-
Notifications
You must be signed in to change notification settings - Fork 522
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
Remove APIClient dependency on the Settings model #3987
Remove APIClient dependency on the Settings model #3987
Conversation
35906c4
to
5302130
Compare
Added a clippy fix! |
When we're in "CLI mode" (not used as a library), can we clean up this error?
In general, I'd like to keep the user-facing output the same before and after this change. Scripts and what not can take dependencies on specific output messages. |
Same for
|
90ac6a8
to
311bdb7
Compare
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.
LGTM.
In order to remove the actual model dependency from Cargo.toml
, it looks like we need model::exec
to move somewhere else. Are you planning to tackle that next?
It's not strictly required here but it would be easy to make the mistake of using more of the model than that, including Settings
which would be broken.
311bdb7
to
b4e10f3
Compare
@bcressey Yeah, Ill add that as part of another PR. I was wondering if it would be a good idea to add those types to the 'modeled-types' package & update any of its references. Like here: https://github.com/bottlerocket-os/bottlerocket/blob/develop/sources/models/modeled-types/src/shared.rs or just add them into APIClient? |
Maybe a shared api-types crate that is just for apiserver & apiclient alignment? modeled-types is somewhat bound up in settings concerns currently, and I foresee it moving to the settings SDK for ease of out-of-tree use. |
Sounds good. Will raise a PR shortly! |
So, #3949 isn't actually closed until that subsequent PR merges... |
Issue number:
Closes
Description of changes:
We removed the APIClient's dependency on the Settings model. We did this for two dependent subcommands,
set
andapply
.apply
command, client-side model verification before serialization was removed.set
command, for key-value pair inputs a new APIserver endpoint /settings/keypair was introduced. This had its benefits as it allowed the current JSON inputs to work as is, but allowed the transfer of key-pair specific logic from the Client to the Server without any major changes i.e no real change in the data-format for the APIServer to be input (key-value or json) aware.Testing done:
Testing was done
locally
& on an aws-ecs-1 AMI instance. Additionally, Testing between existing AMI and new AMI;s with the code changes was done to prove consistent behavior.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.