Skip to content
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

Model the model #824

Merged
merged 2 commits into from
Mar 6, 2020
Merged

Model the model #824

merged 2 commits into from
Mar 6, 2020

Conversation

tjkirch
Copy link
Contributor

@tjkirch tjkirch commented Mar 5, 2020

commit 9c7181a2ac01de120b6da826dc1594565508c835
Author: Tom Kirchner <tjk@amazon.com>
Date:   Tue Mar 3 11:38:37 2020 -0800

Add a top-level model struct and API route

Several benefits here:
* No longer need erased-serde or HashMaps to represent template data
* One API call to see everything the API system knows
  * Useful for curious humans
  * Useful as data source for templating

This also leads us toward the ability to have entirely different models for
different variants, though the API server doesn't support that yet.
commit 3980e543e206e50adadd2cebae3be3a580a23809
Author: Tom Kirchner <tjk@amazon.com>
Date:   Thu Mar 5 13:45:25 2020 -0800

common_migrations: Remove HashMap hack using local struct

This was found when adding a top-level Model struct, which the old comment said
would help with this case, but it actually didn't because we can't use the
model in migrations.  (Would you use the old or the new?)

Instead, we can remove the HashMap by making a single-use struct with a
'settings' key.  This makes it more obvious that only "settings" keys are
supported, so make the docstring reflect that reality too.

🐈 🚶‍♂️

One potential downside that's at least worth considering - this may give the appearance that you can request any given key from the apiserver; since now we have "/" and "/settings" you may expect you can get the tree at "/settings/updates". We couldn't do that without a fancier apiserver.

Testing done:

Launched an instance, it joined my cluster and ran a pod, was running, seemed OK. I used apiclient and saw that the /settings and /os APIs still work, and saw the new top-level API return everything:

{
  "settings": {
    "motd": "Welcome to Bottlerocket!",
...
  "services": {
    "kubernetes": {
      "configuration-files": [
        "kubelet-env",
...
  "configuration-files": {
    "chrony-conf": {
      "path": "/etc/chrony.conf",
...
  "os": {
    "pretty_name": "Bottlerocket OS 0.3.0",
...

This was found when adding a top-level Model struct, which the old comment said
would help with this case, but it actually didn't because we can't use the
model in migrations.  (Would you use the old or the new?)

Instead, we can remove the HashMap by making a single-use struct with a
'settings' key.  This makes it more obvious that only "settings" keys are
supported, so make the docstring reflect that reality too.
Several benefits here:
* No longer need erased-serde or HashMaps to represent template data
* One API call to see everything the API system knows
  * Useful for curious humans
  * Useful as data source for templating

This also leads us toward the ability to have entirely different models for
different variants, though the API server doesn't support that yet.
@tjkirch tjkirch requested review from zmrow and etungsten March 5, 2020 23:16
Copy link
Contributor

@etungsten etungsten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was going to ask if you could test the migration helper but then realized that we've removed all migrations. The changes look right and make sense.

Copy link
Contributor

@zmrow zmrow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this change!

🎉

sources/api/thar-be-settings/src/config.rs Show resolved Hide resolved
@tjkirch tjkirch merged commit c0bd344 into develop Mar 6, 2020
@tjkirch tjkirch deleted the modeled-model branch March 6, 2020 18:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants