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

Added support for MAX_OPEN_FILES environment variable #849 #850

Merged
merged 2 commits into from
Dec 4, 2017

Conversation

alvagante
Copy link
Collaborator

@alvagante alvagante commented Nov 25, 2017

Pull Request Checklist

Description

Related Issue

Fixes # .

Motivation and Context

How Has This Been Tested?

General

  • Update README.md with any necessary configuration snippets

  • New parameters are documented

  • New parameters have tests

  • Tests pass - bundle exec rake validate lint spec

@@ -28,6 +28,8 @@
#
# @param heap_size Value of the HEAP_SIZE environment variable.
#
# @param max_open_files Value of the MAX_OPEN_FILES environment variable.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is the parameter added to three different classes?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I followed the current approach for similar cases, but actually you're right, max_open_files in this case can and should not stay in package.pp. Actually if we split the sensu.erb template used both by /etc/default/sensu and /etc/default/sensu-enterprise we will be able to set ee-only parameters only in enterprise.pp

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need it anywhere except the enterprise class then?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually, we need these params also in init.pp, besides enterprise.pp for how the module is done and tested (all spec tests for the various subclasses, in reality, test on the main class)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need it in the package class then?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Parameters used only in puppet enterprise are not needed in packages class.
FYI just pushed with the code to support other sense ee settings like JAVA_OPTS.
Have to properly test on vagrant yet.
These changes anyway, might be obsoleted by what we decide in #851 .

@alvagante
Copy link
Collaborator Author

@ghoneycutt this should be ready, and even if it might be refactored on the lines on what's under discussion in #852 I'd not wait for it.

@ghoneycutt ghoneycutt merged commit 6cd3670 into sensu:master Dec 4, 2017
@ghoneycutt
Copy link
Collaborator

Thank you!

Released in v2.42.0

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.

2 participants