-
-
Notifications
You must be signed in to change notification settings - Fork 476
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(compat): bare min. to get srv running on 7.x #1040
base: master
Are you sure you want to change the base?
Conversation
Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually? |
Seems to have a little dups here #1021 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks pretty reasonable to me, and I have an interest in ensuring support for 7.x, but am not a maintainer (just a concerned citizen). That said, other than potentially taking on a dependency for users to either a) use the elastic_stack module or b) define a hieradata value in the elastic_stack:: namespace, this seems appropriate for baseline 7.x support.
Looks like the acceptance testing is just missing documentation for the new parameter, though.
manifests/instance.pp:165:parameter_documentation:WARNING:missing documentation for defined type parameter elasticsearch::instance::repo_version
manifests/instance.pp
Outdated
@@ -162,6 +162,7 @@ | |||
Boolean $ssl = false, | |||
Elasticsearch::Status $status = $elasticsearch::status, | |||
Optional[String] $system_key = $elasticsearch::system_key, | |||
Integer $repo_version = $elastic_stack::repo::version, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line will generate a warning if users aren't leveraging the elastic_stack
puppet module, or explicitly passing in a value.
Warning: Unknown variable: 'elastic_stack::repo::version'.
Not sure if this is acceptable, or whether to consider it as taking an implicit dependency on the elastic_stack module
You’re right, I’ve not thought as much on how to handle correctly this version condition. I’ll update this when I have the time ti dig into. Or if someone as suggestions, I’ll consider it. Marked this as WIP |
Is that better like that ? Private var, no mandatory implicit dependency to |
0477a67
to
a05012d
Compare
Fixed jvm.options.erb to add required jvm options for 7.x Fixed jvm.options.erb to change jvm options required only for openjdk 8 Added ES_PATH_DIR instance environment var Fixed log4j2.properties.erb to fix Deprecation WARNINGS Keeped Backward Compatibility
Ping @tylerjl ? |
Apologies for the delays here, I'm trying to get some more resources dedicated to some module updates and maintenance. The short answer is that it gets really challenging to make these sorts of version detection changes since the module is in total control of the jvm.options file - I'm hoping that dropping support for multiple instances will let us manage the jvm.options file on a line-by-line basis, so users will be able to add or remove lines as necessary (that match the format for the version of Elasticsearch they're running) and alleviate the need to populate the entire file at all. |
How could we plan it ? |
ping @tylerjl, I think we need this one way or an other. |
Rel #1032 |
Hi there, Firstly, thank you for taking the time to contribute, and apologies for the radio silence from Elastic on this PR. I'm going to be working on this module over the next few weeks, with an ultimate aim of updating the module to support the elasticsearch 7.x and simplifying the module to make it easier to use. I'll be reviewing all the open issues and PRs over the next few days, and will merge or provide feedback where appropriate. So please hang in there whilst we give this module some TLC. Thanks again. |
Dear @smasa90, thanks for the PR! This is Vox Pupuli Tasks, your friendly Vox Pupuli GitHub Bot. I noticed that your pull request contains merge conflict. Can you please rebase? You can find my sourcecode at voxpupuli/vox-pupuli-tasks |
Fixed jvm.options.erb to add required jvm options for 7.x
Fixed jvm.options.erb to change jvm options required only for openjdk 8
Added ES_PATH_DIR instance environment var
Fixed log4j2.properties.erb to fix Deprecation WARNINGS
Keeped Backward Compatibility but not tested yet
Pull request acceptance prerequisites:
master
fails onDebian 8
andOpenSuse 42
]