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

Add shield functionality #9

Closed
wants to merge 11 commits into from
Closed

Add shield functionality #9

wants to merge 11 commits into from

Conversation

simonclausen
Copy link

@simonclausen simonclausen commented Sep 29, 2016

Related to #7 - I needed it for myself so I just went ahead and wrote the state and templates. Depending on which direction this formula should go, this can also be implemented in an add-on formula.

But then again, as I mentioned in #7, I see shield as core functionality. And, no elasticsearch, no shield :)

P.S. because I did a late separate branch, this also includes the suggested change in #6.

@simonclausen
Copy link
Author

Having worked a bit with these changes, I see that some adjustments need to be made before:

  • Issues when doing first highstate, Shield configs are attempted applied before the plugin is installed
  • Shield config changes does not require a restart of the elasticsearch service.

Copy link

@ryanwalder ryanwalder left a comment

Choose a reason for hiding this comment

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

+1 for adding shield functionality, Made some inline notes.

@@ -1,12 +1,16 @@
include:
- elasticsearch.pkg
- elasticsearch.config
- elasticsearch.shield

Choose a reason for hiding this comment

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

I think shield should be included via a enable_shield (or similar) pillar item as not everyone is going to use shield. Same goes for the watch's on the shield specific files below.

Copy link
Author

Choose a reason for hiding this comment

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

I agree

- template: jinja
- user: root
- require:
- sls: elasticsearch.pkg
Copy link

@ryanwalder ryanwalder Nov 2, 2016

Choose a reason for hiding this comment

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

Could these configs not make use of file.serialize?

Copy link
Author

Choose a reason for hiding this comment

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

Again, I agree that would probably be preferred, cheers

@simonclausen
Copy link
Author

Quick follow up: I have been swamped with other things. I have made some of the mentioned changes in my local running formula, but I do not consider the changes of a sharable quality just yet. Hopefully I can come back with something in December.

@saintx
Copy link

saintx commented Dec 15, 2016

Looking forward to reviewing it when it's ready, @simonclausen!

@simonclausen
Copy link
Author

Ok, so almost half a year later I have time to take this up again. Also a lot of great stuff has been done to this repo. So I am closning this and will be starting over, of course taking the input here with me.

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