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

Fix salt 2019.2 breaking change using yaml filter #50

Merged
merged 2 commits into from
Jul 16, 2019

Conversation

necabo
Copy link
Contributor

@necabo necabo commented Jul 16, 2019

With the salt 2019.2.0 (Fluorine) release there has been a non-backward-compatible change to the yaml renderer.

This PR addresses that change by using the yaml filter where necessary.

Copy link

@vutny vutny left a comment

Choose a reason for hiding this comment

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

Thanks, @necabo
Similar patches were submitted to some repos in Salt Formulas, it's good we have one more formula compatible with latest stable Salt release.

@myii
Copy link
Member

myii commented Jul 16, 2019

Thanks for this PR, @necabo.

@vutny In terms of consistency with the other fixes, would it be appropriate to use | json instead? Or is | yaml required specifically in this case?

@necabo
Copy link
Contributor Author

necabo commented Jul 16, 2019

Or is | yaml required specifically in this case?

I can assure that both | json and | yaml work for the pillar I tested with (example pillar).

@myii
Copy link
Member

myii commented Jul 16, 2019

@necabo Thanks for testing that. There is a long history of changes related to this issue and the consistent option we have selected is to use | json. Of course, there are situations where | json doesn't work and we will use | yaml instead. Unless that's the case here, would you mind changing this PR to use | json instead? That will help us come back to these, when the eventual solution of | tojson needs to be considered for implementation. Refer back to saltstack-formulas/consul-formula#40 (comment) and the linked content there for more info.

@myii myii merged commit bafb2e9 into saltstack-formulas:master Jul 16, 2019
@myii
Copy link
Member

myii commented Jul 16, 2019

@necabo @vutny Merged, thanks to you both.

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