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

Allow configuring munge via this module #23

Merged
merged 1 commit into from
Sep 17, 2021

Conversation

martijndegouw
Copy link
Contributor

No description provided.

@treydock
Copy link
Owner

This would cause conflicts and duplicate resource issues for anyone who specifies include munge before including the SLURM module or anyone who uses class { 'munge': ... } inside a profile. One way I've made these kind of include vs class declaration choices in modules is allow a site to change the behavior with a parameter. So I'd recommend something like this:

Boolean $manage_munge_class = false

Then inside the new slurm::common::munge do this:

if $slurm::manage_munge_class {
  class { 'munge':
   ...
   }
} else {
  include munge
}

Also you will need to update slurmd.pp, slurmdbd.pp and slurmctld.pp to use the new common munge class rather than direct munge include.

@martijndegouw
Copy link
Contributor Author

Thanks a lot for the feedback. I updated the PR accordingly.

@@ -226,6 +226,11 @@
$slurmd_user = 'root',
$slurmd_user_group = 'root',

# Munge key
Boolean $manage_munge = false,
Copy link
Owner

Choose a reason for hiding this comment

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

These 3 new parameters will need documentation at the top of the file otherwise the puppet-lint tests will fail.

Comment on lines 50 to 52
# @manage_munge
# @munge_key_source
# @munge_key_content
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
# @manage_munge
# @munge_key_source
# @munge_key_content
# @param manage_munge
# @param munge_key_source
# @param munge_key_content

Copy link
Contributor Author

Choose a reason for hiding this comment

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

aaaaaaaaargh

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It must be friday...

@treydock
Copy link
Owner

Go ahead and merge master branch or rebase against master to pick up fix for acceptance tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants