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

render config pillar as config file #12

Merged
merged 4 commits into from
Nov 14, 2016

Conversation

blbradley
Copy link
Contributor

Hello!

I'd like to reduce the complexity of the formula via file.serialize to render the config pillar directly to the Elasticsearch YAML config file. This works excellently since Elasticsearch comes with great defaults.

The only issue here will be maintaining backward compatibility with custom_options. I haven't done there here yet but would like to try.

@blbradley
Copy link
Contributor Author

@martinhoefling @kpostrup ping!

@aboe76
Copy link
Member

aboe76 commented Nov 10, 2016

@blbradley, I don't see a simple solution than to break backward compatibility....

@blbradley
Copy link
Contributor Author

blbradley commented Nov 11, 2016

@aboe76 elasticsearch:config:custom_options key/values can be extracted and merged with elasticsearch:config with some careful Jinja. I see that as the only way to maintain backwards compatibility for some time.

Whether that is a good idea or not is another matter. Doing this will break someone's setup now or later, but a deprecation warning for some time would be ideal. On the other hand, users should be keeping there own forks and expecting breaking changes anytime they upgrade (which has been my feelings in practice).

@blbradley
Copy link
Contributor Author

I should also be explict: I'm fine with breaking changes.

@blbradley
Copy link
Contributor Author

Rebased test suite and added some conditionals to pass.

@blbradley
Copy link
Contributor Author

@aboe76 If you choose to merge, please 'Squash and Merge'.

@blbradley
Copy link
Contributor Author

It would be wise to add another set of tests that verify this works when elasticsearch:config pillar is not empty.

@aboe76
Copy link
Member

aboe76 commented Nov 14, 2016

@blbradley so we are dropping elasticsearch.config['custom_options'] %} shouldn't this be mentioned in the README? and dropped from the pillar.example?

@blbradley
Copy link
Contributor Author

@aboe76 You are correct. I'll look to see how other formulas show deprecation warnings in documentation.

@aboe76
Copy link
Member

aboe76 commented Nov 14, 2016

@blbradley maybe we should ping @gravyboat maybe he has some nice info on how to make an option pillar option deprecated...

@blbradley
Copy link
Contributor Author

The only thing I could find was some formulas having a Notes section.

@aboe76
Copy link
Member

aboe76 commented Nov 14, 2016

@blbradley than let's do that.

@blbradley
Copy link
Contributor Author

Feel free to add to the note if you feel it is not clear enough.

@aboe76 aboe76 merged commit fcbf7ee into saltstack-formulas:master Nov 14, 2016
@aboe76
Copy link
Member

aboe76 commented Nov 14, 2016

Ok done, merged it.

@blbradley blbradley deleted the config-pillar-to-yaml branch November 14, 2016 19:32
@blbradley
Copy link
Contributor Author

Thanks for the squash!

@simonclausen
Copy link

Good to see this change made :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants