Skip to content

Remove cheetah from templater (SC-964)#1416

Merged
holmanb merged 3 commits into
canonical:mainfrom
holmanb:holmanb/cheetah-go-brrr
Apr 29, 2022
Merged

Remove cheetah from templater (SC-964)#1416
holmanb merged 3 commits into
canonical:mainfrom
holmanb:holmanb/cheetah-go-brrr

Conversation

@holmanb
Copy link
Copy Markdown
Member

@holmanb holmanb commented Apr 28, 2022

Remove cheetah template code

- cheetah was dropped as a requirement in 17.1
- we still have cheetah template code
- cheetah test fails
- no known users - remove rather than fix

@holmanb
Copy link
Copy Markdown
Member Author

holmanb commented Apr 28, 2022

The failure: cheetah-fail.log

Copy link
Copy Markdown
Contributor

@TheRealFalcon TheRealFalcon left a comment

Choose a reason for hiding this comment

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

We should be able to clear out this file too:
https://github.com/canonical/cloud-init/blob/main/tests/unittests/__init__.py/

Verified there's no templates using cheetah anywhere in the code base and that it's impossible to use a cheetah template as user data. It is still theoretically possible to include a cheetah template somewhere in /etc/cloud (cloud-init devel render can still render it), but given that there were multiple bugs almost 10 years ago complaining about how old and unsupported cheetah is, jinja is ubiquitous and has been for years, and we have no reference to it anywhere in our docs, I think the regression risk is small enough that we can just drop it.

@blackboxsw , do you agree?

@holmanb holmanb changed the title Remove cheetah from templater Remove cheetah from templater (SC-964) Apr 29, 2022
@blackboxsw
Copy link
Copy Markdown
Collaborator

Yes good suggestion here. I agree that this should be dropped. And per @TheRealFalcon's concern about someone potentially adding a ##template: cheetah type file in /etc/cloud/cloud.cfg.d I think that is impossible. Our config merger doesn't currently use the template handlers when merging config types. it expects straight cloud-config YAML or overrides to base system config keys in /etc/cloud/cloud.cfg. Trying to supply template files doesn't work currenty for any type of template we'd want to inject.

What we get is an error along the lines of the following (confirmed with either jinja or cheetah templates)

2022-04-29 20:14:55,007 - util.py[WARNING]: Failed loading yaml blob. Invalid format at line 3 column 2: "while scanning for the next token

Generally speaking though, I think it would be a pretty useful feature to allow templates in /etc/cloud/cloud.cfg.d. We should discuss this and decide whether we can carve out some time to provide that feature (it's been requested a few times in drive-by questions in IRC).

@blackboxsw
Copy link
Copy Markdown
Collaborator

The issue we'd have to solve if we wanted to support rendering jinja templates from /etc/cloud/cloud.cfg.d is how or whether to stage the rendering of that content after the datasource is detected. Only after the datasource is detected do we have the most used/valuable instance-data variables for use in jinja templates from the cloud metadata information.

Copy link
Copy Markdown
Collaborator

@blackboxsw blackboxsw left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the view to remove antiquated content/functionality.

@TheRealFalcon
Copy link
Copy Markdown
Contributor

Oh, I assumed templating was already a thing for /etc/cloud. If it's not, I'm not sure I want to support it 😄 . I'm assuming the main reason people are asking is so they can shove more userdata into /etc/cloud/cloud.cfg, which I don't think we should be encouraging. Would there be other use cases for templating /etc/cloud stuff?

@holmanb
Copy link
Copy Markdown
Member Author

holmanb commented Apr 29, 2022

@blackboxsw @TheRealFalcon Thanks for the reviews!

@holmanb holmanb merged commit b4746b6 into canonical:main Apr 29, 2022
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