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

feat: prevent review app specs from being jinja rendered #410

Merged
merged 7 commits into from
May 28, 2024

Conversation

artsyjian
Copy link
Contributor

Solves #408

A review app spec does not have to be Jinja rendered because any Jinja template was already rendered when the spec was created during hokusai review_app setup. During spec creation, Hokusai copied Staging spec and rendered Jinja templates.

When Staging spec has Jinja template escaped with {% raw %}, the double rendering causes hokusai review_app create to break.

This PR proposes to eliminate the second rendering. To achieve that, a render_template flag is introduced and passed down the call chain. We default it to True so there's no impact on non-review-app commands (e.g. hokusai staging ..., hokusai production ...). Review app commands set it to False, so that the review app spec is not rendered.

This PR also replaces legacy test/unit/test_yaml_spec.py with a Pytest-format version that is more comprehensive.

@artsyjian artsyjian requested a review from a team May 24, 2024 17:10
Copy link
Contributor

@joeyAghion joeyAghion left a comment

Choose a reason for hiding this comment

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

If I understand correctly, this means review apps' specs will be "rendered" once at create time, while standard deployments will be rendered each time they're needed. Do we need to be careful about changing template inputs, since review apps won't get the updates? (In my memory, we only change those inputs very, very rarely.)

@artsyjian
Copy link
Contributor Author

If I understand correctly, this means review apps' specs will be "rendered" once at create time...

Correct. hokusai review_app setup leads to YamlSpec.to_file() being called, with Staging's spec passed in as source_file, so it's Staging's spec that gets rendered. That rendered spec then gets written as Review App spec file.

@artsyjian
Copy link
Contributor Author

Do we need to be careful about changing template inputs, since review apps won't get the updates?

The Review App spec file created by hokusai review app setup does not contain any template (other than those escaped by {{ %raw }}. And Hokusai itself does not add any template:

(hokusai) artsy:hokusai jxu$ git grep '{{' | grep -v 'hokusai/templates' | grep -v fixture | grep -v doc
Binary file hokusai.jpg matches

So there should be nothing to render at the time of hokusai review_app create.

@joeyAghion joeyAghion merged commit ace5ad5 into main May 28, 2024
4 checks passed
@joeyAghion joeyAghion deleted the artsyjian/review branch May 28, 2024 16:57
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

2 participants