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(compat): bare min. to get srv running on 7.x #1040

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

smasa90
Copy link

@smasa90 smasa90 commented Jun 27, 2019

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:

  • Signed the CLA (if not already signed)
  • Rebased/up-to-date with base branch
  • Tests pass [ except recurrent master fails on Debian 8 and OpenSuse 42 ]

@elasticmachine
Copy link

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?

@smasa90
Copy link
Author

smasa90 commented Jun 27, 2019

Seems to have a little dups here #1021
Didn't see the PR before pushing this one.

Copy link

@ernstae ernstae 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 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

@@ -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,
Copy link

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

@smasa90 smasa90 changed the title fix(compat): bare min. to get srv running on 7.x WIP: fix(compat): bare min. to get srv running on 7.x Jun 27, 2019
@smasa90
Copy link
Author

smasa90 commented Jun 27, 2019

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

@smasa90 smasa90 changed the title WIP: fix(compat): bare min. to get srv running on 7.x fix(compat): bare min. to get srv running on 7.x Jul 2, 2019
@smasa90
Copy link
Author

smasa90 commented Jul 2, 2019

Is that better like that ? Private var, no mandatory implicit dependency to elastic_stack module. Default to false if no version is provided nor in elastic_stack::repo::version nor in elasticsearch::version. Can default to true when community will be ready to default to 7.x with Puppet modules.

@smasa90 smasa90 force-pushed the master branch 3 times, most recently from 0477a67 to a05012d Compare July 4, 2019 00:01
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
@smasa90
Copy link
Author

smasa90 commented Jul 4, 2019

Ping @tylerjl ?

@tylerjl
Copy link
Contributor

tylerjl commented Jul 9, 2019

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.

@smasa90
Copy link
Author

smasa90 commented Jul 10, 2019

How could we plan it ?
A solution can be to copy default jvm.options file on each instance and manage it line by line

@sticky-note
Copy link

ping @tylerjl, I think we need this one way or an other.
If this module is not maintained any more, on which provisioner can we turn ourselves to get a maintained module ?

@NITEMAN
Copy link
Contributor

NITEMAN commented Oct 18, 2019

Rel #1032
Any updates on this?

@smasa90 smasa90 mentioned this pull request Dec 18, 2019
5 tasks
@fatmcgav
Copy link
Contributor

fatmcgav commented Feb 4, 2020

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.

@vox-pupuli-tasks
Copy link

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

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.

7 participants