sources: update to handlebars v4 #1707
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
webern
approved these changes
Aug 10, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of changes:
I left handlebars out of #1683 because there were fairly substantial changes required. The relevant change in handlebars is this which means
Empty lines around block helpers are now stripped. Essentially, before, we had to use~in many places to remove whitespace before/after{{}}blocks, even where they're not required in "standard" mustache. There were several issues with the initial upstream change, so we had to wait for the 4.1 release for fixes.So, you'll see a lot of
~removal in the diff. The only significant change in templates is theelseclause added to theproxy-envconfiguration. handlebars v4 (in strict mode) no longer allowseachif the referenced variable is missing, unless there's anelse. Everywhere else, we haveeachin a conditional that checks that the referenced variable is populated. (Outside of templates, they changed an error variant name, but that's it.)The only place I left the
~is in kubernetes-ca-crt files, since I think they would be invalid with any whitespace, and we wouldn't have any other data in those files anyway.Testing done:
Unit tests pass. k8s and ecs AMIs are healthy.
There's a lot of potential for small changes in behavior here, so I made a little pipeline to confirm there were no changes in rendered output. I modified thar-be-settings to save rendered files locally and not restart services, which wouldn't exist on my laptop. I created a default data store, rendered before and after, and diffed the directories. I then set clusters of settings, going through all k8s settings, registry mirrors, and proxy settings, to cover everything affected in the templated files. I set settings.kubernetes.authentication-mode to aws and tls and checked both, since they cause different branches in templates. No differences.
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.