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

thar-be-settings: allow formatting maps into configuration files #408

Merged
merged 1 commit into from
Oct 15, 2019

Conversation

tjkirch
Copy link
Contributor

@tjkirch tjkirch commented Oct 14, 2019

The initial use case is joining together node labels and taints, which are
stored in a map but needed in our configuration in key1=value1,key2=value2
format.

This change has a few pieces:

  • t-b-s will fetch all settings instead of just ones found in templates.
    • This allows us to reference prefixes in templates, which would fail before
      because t-b-s would try to fetch them from the API as specific keys.
  • Remove the code that handled finding settings from templates since we fetch
    them all.
  • Add a join_map template helper that lets you format a map for display by
    joining keys/values and then joining together those key/value pairs.

Testing done:

Unit tests pass.

On an instance, I added # {{ join_map "=" "," settings.host-containers.admin }} to the updog-toml template (just to have a spot) and saw it generate properly:

Oct 14 22:50:15.826 DEBUG thar_be_settings::config: Rendering updog-toml
Oct 14 22:50:15.826 TRACE thar_be_settings::helpers: Starting join_map helper
Oct 14 22:50:15.826 TRACE thar_be_settings::helpers: Template name: updog-toml
Oct 14 22:50:15.826 TRACE thar_be_settings::helpers: Number of params: 3
Oct 14 22:50:15.827 TRACE thar_be_settings::helpers: Character used to join keys to values: =
Oct 14 22:50:15.827 TRACE thar_be_settings::helpers: Character used to join pairs: ,
Oct 14 22:50:15.827 TRACE thar_be_settings::helpers: Map to join: {"enabled": Bool(true), "source": String("328549459982.dkr.ecr.us-west-2.amazonaws.com/thar-admin:v0.1"), "superpowered": Bool(true)}
Oct 14 22:50:15.827 TRACE thar_be_settings::helpers: Joined output: enabled=true,source=328549459982.dkr.ecr.us-west-2.amazonaws.com/thar-admin:v0.1,superpowered=true
Oct 14 22:50:15.827 TRACE thar_be_settings::config: Rendered configs: [RenderedConfigFile { path: "/etc/updog.toml", rendered: "metadata_base_url = \"https://d25d9m6x9pxh9h.cloudfront.net/45efedef4afe/metadata/\"\ntarget_base_url = \"https://d25d9m6x9pxh9h.cloudfront.net/45efedef4afe/targets/\"\nseed = 401\n# enabled=true,source=328549459982.dkr.ecr.us-west-2.amazonaws.com/thar-admin:v0.1,superpowered=true\n" }]
bash-5.0# cat /etc/updog.toml 
metadata_base_url = "https://d25d9m6x9pxh9h.cloudfront.net/45efedef4afe/metadata/"
target_base_url = "https://d25d9m6x9pxh9h.cloudfront.net/45efedef4afe/targets/"
seed = 401
# enabled=true,source=328549459982.dkr.ecr.us-west-2.amazonaws.com/thar-admin:v0.1,superpowered=true

I'm hoping @etungsten can test this for simplifying #390.

@tjkirch
Copy link
Contributor Author

tjkirch commented Oct 14, 2019

@etungsten found that I had an incorrect usage example in the docstring, thanks!

@tjkirch
Copy link
Contributor Author

tjkirch commented Oct 14, 2019

(I'm working on testing another change that's required to make this useful - see #390 (comment) . Also note that the helper has to be registered in thar-be-setting's src/template.rs, the same as the base64_decode helper is registered; I didn't do that in this PR, but I should if we ship it.)

@tjkirch
Copy link
Contributor Author

tjkirch commented Oct 14, 2019

This push changes thar-be-settings to allow referencing prefixes, so that we can actually reference maps in a template. See new description/testing.

@tjkirch tjkirch requested a review from zmrow October 14, 2019 23:12
@tjkirch tjkirch changed the title Add join_map helper for thar-be-settings templates thar-be-settings: allow formatting maps into configuration files Oct 14, 2019
@tjkirch tjkirch requested a review from bcressey October 14, 2019 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.

Awesome, I tested with node-labels and node-taints settings and it works!

@tjkirch
Copy link
Contributor Author

tjkirch commented Oct 15, 2019

This push disallows null values; we don't have a use for them and I think they'd be a little confusing, so let's block them for now.

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.

Nice work! Deleting code is so satisfying isn't it?

Other than 1 nit I'm absolutely ok with foregoing, Shipit.

🗡

The initial use case is joining together node labels and taints, which are
stored in a map but needed in our configuration in `key1=value1,key2=value2`
format.

This change has a few pieces:
* t-b-s will fetch all settings instead of just ones found in templates.
  * This allows us to reference prefixes in templates, which would fail before
    because t-b-s would try to fetch them from the API as specific keys.
* Remove the code that handled finding settings from templates since we fetch
  them all.
* Add a `join_map` template helper that lets you format a map for display by
  joining keys/values and then joining together those key/value pairs.
@tjkirch
Copy link
Contributor Author

tjkirch commented Oct 15, 2019

This push adds another parameter to the helper so the user can specify their desired behavior if the map is missing. @etungsten found that the template was failing to render if node-labels or node-taints wasn't set, which is unacceptable; our options are to conditionalize every template where this could happen, which adds a fair amount of template overhead, or make the helper handle it, which we chose. I chose a descriptive "fail-if-missing" or "no-fail-if-missing" argument rather than a boolean so you don't have to understand the details of the templating system to understand the behavior of the helper in a template.

@zmrow
Copy link
Contributor

zmrow commented Oct 15, 2019

I'm hesitant to be ok with this.

I'm not sure I buy the "overhead" argument when it comes to conditionals in templates; in this case the conditional will only live in one template.. I think conditionals in templates are very clear and straightforward and leave the rendering logic to the template engine. It also eliminates helper parameters which are difficult for a user to find without digging into our code.

That being said - this is a very custom use case unique to this particular arm of the code and not something that needs to be widely used.

For now, I'm against it. I'll let other folks chime in.

@tjkirch
Copy link
Contributor Author

tjkirch commented Oct 15, 2019

I'm not sure I buy the "overhead" argument when it comes to conditionals in templates

etungsten tried it with conditionals and we both thought it turned out pretty bad. I'll send you an example.

It also eliminates helper parameters which are difficult for a user to find without digging into our code.

That's why I went with the obvious-as-possible naming of the parameter. If anyone needs to use this helper, they'll probably copy an existing use, and that parameter name is as obvious as any name I could think of...

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.

Tested again both with (node-labels & node-taints) set and none set and it works for both!

@tjkirch
Copy link
Contributor Author

tjkirch commented Oct 15, 2019

Discussed a bit more with @zmrow offline and he was OK with continuing with this approach; it's easy to change later if desired, it doesn't affect the user experience.

@tjkirch tjkirch merged commit 2680258 into develop Oct 15, 2019
@tjkirch tjkirch deleted the join-map-helper branch October 15, 2019 19:25
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.

4 participants