-
Notifications
You must be signed in to change notification settings - Fork 115
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
Use raw rendered output #454
Conversation
@@ -3,7 +3,7 @@ kind: Pod | |||
metadata: | |||
name: unmanaged-pod-1-<%= deployment_id %> | |||
annotations: | |||
kubernetes-deploy.shopify.io/timeout-override: "60s" | |||
kubernetes-deploy.shopify.io/timeout-override: 60s |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I should leave this file alone and just fix the expected output in the tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't really matter to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approach seems valid. Let's make sure to have both regression tests for the problem loading caused and tests showing that the header handling is working as expected.
--- | ||
supports_partials: "yep" | ||
|
||
# This is valid |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not stripping the comments is a nice side-effect
@@ -3,7 +3,7 @@ kind: Pod | |||
metadata: | |||
name: unmanaged-pod-1-<%= deployment_id %> | |||
annotations: | |||
kubernetes-deploy.shopify.io/timeout-override: "60s" | |||
kubernetes-deploy.shopify.io/timeout-override: 60s |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't really matter to me
Tests are fixed up. I apparently had a brain-fart last night when I was writing them. |
The code LGTM~ |
|
What are you trying to accomplish with this PR?
Be able to detect invalid yaml. Specifically cases where the document separator
---
is missing in an erb loop.How is this accomplished?
Currently we load and dump yaml documents before outputing them. Unfortunately, when the parser encounters the same key multiple times its uses the last one. Instead output the raw rendered yaml so a tool like
yaml lint
can be used. We still load and dump for some level of syntax checking.What could go wrong?
The yaml output is different and that leads to issues. I still need to figure out what's up with test_render_task_with_partials_and_bindings
it seems like the yaml is getting rendered multiple times ...