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

Do not add doc separator when render result is blank #467

Merged
merged 2 commits into from
Apr 22, 2019

Conversation

KnVerey
Copy link
Contributor

@KnVerey KnVerey commented Apr 17, 2019

What are you trying to accomplish with this PR?

Fix a regression from #454. It is causing the template validator for my Web PR to fail because a file entire content is rendered conditionally. Before that PR, the render result was an empty file, which I think makes sense. Now it instead contains an empty YAML doc. We should not be adding that header.

How is this accomplished?
Add a regression test and a presence check on the render result.

What could go wrong?
There's a reason I'm not thinking of why having an extra header could be a good thing?

@Shopify/cloudx

Copy link
Contributor

@RyanBrushett RyanBrushett left a comment

Choose a reason for hiding this comment

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

This looks great IMO. I’m in agreement that an empty file makes more sense than a file with an empty yaml doc.

👍 Good eye

Copy link
Contributor

@timothysmith0609 timothysmith0609 left a comment

Choose a reason for hiding this comment

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

🚢

lib/kubernetes-deploy/render_task.rb Outdated Show resolved Hide resolved
@KnVerey KnVerey merged commit 165bd96 into master Apr 22, 2019
@KnVerey KnVerey deleted the no_extra_yaml_docs branch April 22, 2019 17:55
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

3 participants