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.

Sign up for GitHub

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

Jump to bottom

Fix strict variables #102

Merged
merged 1 commit into from
May 20, 2017
Merged

Fix strict variables #102

merged 1 commit into from
May 20, 2017

Conversation

coreone
Copy link
Contributor

@coreone coreone commented May 13, 2017

  • Fix strict variables for defaults in params.pp
  • Fix versioncmp for Puppet 4 in params.pp
  • Test for undefined variable in snmp.conf.erb
  • Add missing trap_service_name for Debian in params.pp

@coreone coreone mentioned this pull request May 13, 2017
Closed
@coreone
Copy link
Contributor Author

coreone commented May 13, 2017

Fixes Travis errors from #97

@coreone
Copy link
Contributor Author

coreone commented May 13, 2017

@gusnuf @razorsedge refactored to comply with CONTRIBUTING. I will re-submit another PR for the master/agentx changes once this PR is merged.

@coreone
Copy link
Contributor Author

coreone commented May 13, 2017

If support for top-scope variables was no longer needed, something like #85 would make much more sense since it makes params.pp much cleaner. If that were the case, I would rebase this and just fix the RedHat-specific variables and the versioncmp issue in params.pp.

@razorsedge razorsedge added the enhancement New feature or request label May 17, 2017
@razorsedge razorsedge self-assigned this May 17, 2017
@razorsedge razorsedge self-requested a review May 17, 2017 13:53
razorsedge
razorsedge requested changes May 17, 2017
View reviewed changes
Copy link
Contributor

@razorsedge razorsedge left a comment

Choose a reason for hiding this comment

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

I have no idea if people are using top-scope variables, so I am leaning towards keeping them for now. If you can help me understand if metadata.json's puppetlabs/stdlib dependency needs to be bumped for getvar and versioncmp that would help get this merged.

manifests/params.pp Outdated
@@ -284,9 +362,11 @@

case $::osfamily {
'RedHat': {
if $::operatingsystemmajrelease { # facter 1.7+
$osmr = getvar('::operatingsystemmajrelease')
$lsbmdr = getvar('::lsbmajdistrelease')
Copy link
Contributor

Choose a reason for hiding this comment

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

This shouldn't be necessary as ::operatingsystemmajrelease and ::lsbmajdistrelease are facts.

manifests/params.pp
@@ -302,7 +382,7 @@
$service_config_perms = '0600'
}
default: {
if $majdistrelease <= '5' {
if versioncmp($majdistrelease, '5') <= 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

What version of puppetlabs/stdlib (I am assuming this is in puppetlabs/stdlib) has versioncmp? Do we need to bump dependencies in metadata.json?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

versioncmp is part of Puppet since at least 3.5:

https://docs.puppet.com/puppet/3.5/function.html#versioncmp

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Additionally, getvar is in stdlib 2.3.0, so metadata.json should not need editing for that either:

https://github.com/puppetlabs/puppetlabs-stdlib/tree/2.3.0/lib/puppet/parser/functions

Copy link
Contributor

Choose a reason for hiding this comment

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

Excellent.

manifests/params.pp Outdated
@@ -358,6 +438,7 @@
$client_package_name = 'snmp'
$client_config = '/etc/snmp/snmp.conf'

$trap_service_name = 'snmptrapd'
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this have to do with strict vars? I believe it is missing because tested versions of Debian do not have it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In init.pp, $trap_service_name is defaulted to $snmp::params::trap_service_name, so if that is happening on Debian, the default will not exist in params, which can lead to warnings in P4 about undefined variables.

Copy link
Contributor

Choose a reason for hiding this comment

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

In that case, lets set it to empty ''.

@coreone
Copy link
Contributor Author

coreone commented May 17, 2017

@razorsedge I believe I answered everything inline...

@coreone
Copy link
Contributor Author

coreone commented May 18, 2017

@razorsedge should be all set. Set it to empty string instead of snmptrapd

@coreone
* Fix strict variables for defaults in params.pp
7901b28 
* Fix versioncmp for Puppet 4 in params.pp
* Test for undefined variable in snmp.conf.erb
* Add missing trap_service_name for Debian in params.pp
@coreone
Copy link
Contributor Author

coreone commented May 18, 2017

empty string 👎 , undef 👍

@razorsedge razorsedge merged commit 7901b28 into voxpupuli:develop May 20, 2017
@coreone coreone deleted the strict_var_fix branch May 20, 2017 11:14
This was referenced May 28, 2017
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@razorsedge razorsedge razorsedge approved these changes

Assignees

@razorsedge razorsedge

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@coreone @razorsedge