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.

Sign up for GitHub

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

Jump to bottom

Use raw rendered output #454

Merged
merged 5 commits into from
Mar 27, 2019
Merged

Use raw rendered output #454

merged 5 commits into from
Mar 27, 2019

Conversation

dturn
Copy link
Contributor

@dturn dturn commented Mar 26, 2019 •
edited
Loading

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 ...

@dturn dturn changed the title Use raw rendered output [wip] Use raw rendered output Mar 26, 2019
dturn
dturn commented Mar 26, 2019
View reviewed changes
test/fixtures/hello-cloud/unmanaged-pod-1.yml.erb
@@ -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
Copy link
Contributor Author

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?

Copy link
Contributor

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

KnVerey
KnVerey reviewed Mar 26, 2019
View reviewed changes
Copy link
Contributor

@KnVerey KnVerey left a 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.

lib/kubernetes-deploy/render_task.rb Outdated Show resolved Hide resolved
test/integration/render_task_test.rb Show resolved Hide resolved
test/integration/render_task_test.rb
---
supports_partials: "yep"

# This is valid
Copy link
Contributor

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

@dturn dturn changed the title [wip] Use raw rendered output Use raw rendered output Mar 26, 2019
@dturn dturn requested a review from jessie-JNing March 26, 2019 23:57
KnVerey
KnVerey reviewed Mar 27, 2019
View reviewed changes
test/integration/render_task_test.rb Show resolved Hide resolved
test/integration/render_task_test.rb Outdated Show resolved Hide resolved
test/fixtures/hello-cloud/unmanaged-pod-1.yml.erb
@@ -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
Copy link
Contributor

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

test/fixtures/invalid-partials/duplicate-keys.yml Outdated Show resolved Hide resolved
@dturn
Copy link
Contributor Author

dturn commented Mar 27, 2019

Tests are fixed up. I apparently had a brain-fart last night when I was writing them.

@jessie-JNing
Copy link
Contributor

The code LGTM~
If there's a 🎩 that proves yamllint capture the duplicate keys, will be great!

jessie-JNing
jessie-JNing approved these changes Mar 27, 2019
View reviewed changes
test/integration/render_task_test.rb Outdated Show resolved Hide resolved
@dturn
Copy link
Contributor Author

dturn commented Mar 27, 2019 

➜  kubernetes-deploy git:(render-raw) ✗ be exe/kubernetes-render  --template-dir test/fixtures/partials duplicate-keys.yml.erb | yamllint -           
[INFO][2019-03-27 11:55:10 -0700]	
[INFO][2019-03-27 11:55:10 -0700]	---------------------------------Phase 1: Initializing render task----------------------------------
[INFO][2019-03-27 11:55:10 -0700]	Validating configuration
[INFO][2019-03-27 11:55:10 -0700]	
[INFO][2019-03-27 11:55:10 -0700]	-----------------------------------Phase 2: Rendering template(s)-----------------------------------
[INFO][2019-03-27 11:55:10 -0700]	Rendering duplicate-keys.yml.erb ...
[INFO][2019-03-27 11:55:10 -0700]	Rendered duplicate-keys.yml.erb
[INFO][2019-03-27 11:55:10 -0700]	
[INFO][2019-03-27 11:55:10 -0700]	------------------------------------------Result: SUCCESS-------------------------------------------
[INFO][2019-03-27 11:55:10 -0700]	Successfully rendered 1 template(s)
[INFO][2019-03-27 11:55:10 -0700]	
stdin
  3:1       error    duplication of key "key1" in mapping  (key-duplicates)
  4:1       error    duplication of key "key1" in mapping  (key-duplicates)

@dturn dturn merged commit 9ccce22 into master Mar 27, 2019
@dturn dturn deleted the render-raw branch March 27, 2019 20:34
@KnVerey KnVerey mentioned this pull request Apr 17, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KnVerey KnVerey KnVerey approved these changes

@jessie-JNing jessie-JNing jessie-JNing approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@dturn @jessie-JNing @KnVerey