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

Migrate to schnauzer v2 for generated settings #3316

Conversation

cbgbt
Copy link
Contributor

@cbgbt cbgbt commented Aug 3, 2023

NOTE This is against the ootb-settings-extension feature branch.

Issue number:
#3133

Description of changes:
This change uses the new schnauzer::v2 template renderer to replace all template-based settings generators in Bottlerocket.

Specifically, it:

  • Implements schnauzer-v2 settings generator binary
  • Modifies sundog to use a POSIX-shell-compatible lexer to tokenize settings generators
  • Modifies schnauzer-based settings generators to use schnauzer-v2

Thoughts on Migrations
We overwrite the existing setting-generator for schnauzer-based generators, but leave the templates intact in the API for existing customers. This should enable rollbacks to succeed. Since the new setting generator is semantically identical to the previous one, we don't need to re-render the settings.

What's Still Missing
See #3133 for the list of tasks to complete this migration.

Testing done:

  • All unit/integration tests pass
  • Preliminary testing of aws-k8s and aws-ecs variants
  • Migration testing (up & down)

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.

@cbgbt cbgbt force-pushed the schnauzer-generator-migrations branch from 5bfd8e7 to d527cf1 Compare August 3, 2023 22:11
@cbgbt
Copy link
Contributor Author

cbgbt commented Aug 3, 2023

^ I added a commit to this feature branch to enable GitHub workflows.

@cbgbt cbgbt force-pushed the schnauzer-generator-migrations branch 2 times, most recently from 02d5c8d to 48d365b Compare August 3, 2023 23:00
@cbgbt cbgbt marked this pull request as ready for review August 3, 2023 23:41
@cbgbt cbgbt force-pushed the schnauzer-generator-migrations branch from 48d365b to 97b8ed4 Compare August 3, 2023 23:49
},
),
(
"weird-extension@but_valid1.23(1, 2, 3)",
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't feel strongly about this, but for CLI input I tend to avoid anything that might get mangled or misinterpreted if pasted into a prompt or script. Parens and whitespace jump out at me as slightly problematic.

Would something like weird-extension@but_valid1.23+1,2,3 ("add the 1, 2, and 3 helpers from this version of this extension") support the requirements?

Copy link
Contributor Author

@cbgbt cbgbt Aug 4, 2023

Choose a reason for hiding this comment

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

It should, though I'm conflicted. I've been thinking about this syntax since I submitted this PR.

The requires line is meant to basically replicate the functionality of frontmatter. One thing it doesn't do here is allow the optional = true field, which is probably fine; however, I don't want to be trapped in a corner of never being able to support it.

I've been thinking we may need something like extension@version(helpers=[helper1, helper2, ...]) so that we can add other named parameters if needed later. Of course, if the time comes, a regex would make for a terrible parser, but that's a bridge for another time.

In spite of all of this, we could probably still use different separators to avoid parens and whitespace, maybe something like:

extension@version+helpers:1,2,3+optional:true

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, on the other hand, it's extremely unlikely that the templates themselves will be CLI-friendly, they almost always contain whitespace and curly brace characters.

I'm erring on continuing to use the parens (I personally find it more readable) but adding the named arguments.

//! to resolve references to settings extensions and the requested helpers.
//!
//! Most users will want to use `schnauzer::BottlerocketTemplateImporter`, which uses settings extensions to resolve
//! settings and helper data; however, custom `TemplateImporter` implementrations can be used as well.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: "implementations"

//! schnauzer was originally written to render simpler templates which always had the full scope of Bottlerocket
//! settings and helper functions available to them, making it incompatible with the concept of Out-of-Tree Builds.
//!
//! The original schnauzer library deprecated, but continues to be made available under `schnauzer::v1` until it can
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: "is deprecated"

@cbgbt cbgbt force-pushed the schnauzer-generator-migrations branch from 97b8ed4 to 285d0a6 Compare August 8, 2023 18:18
@cbgbt
Copy link
Contributor Author

cbgbt commented Aug 8, 2023

  • Modified the CLI helper specification syntax to be more future-proof
  • Fixed an issue where migrations would be built for all variants when they actually only apply to a subset.

This change also uses a POSIX-shell-like lexer to tokenize
setting-generators in sundog, rather than the previous approach of
splitting on whitespace.
@cbgbt cbgbt force-pushed the schnauzer-generator-migrations branch from 285d0a6 to b667a91 Compare August 8, 2023 18:24
@cbgbt
Copy link
Contributor Author

cbgbt commented Aug 8, 2023

  • Fixes README issues pointed out by @bcressey

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.

🚀

@cbgbt cbgbt merged this pull request into bottlerocket-os:ootb-settings-extensions Aug 9, 2023
42 checks passed
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.

3 participants