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

Add $snmpv2_enable parameter #136

Merged
merged 1 commit into from
Sep 23, 2018
Merged

Add $snmpv2_enable parameter #136

merged 1 commit into from
Sep 23, 2018

Conversation

hdep
Copy link
Contributor

@hdep hdep commented Sep 15, 2018

Same PR as #128 with fixes as requested.

@bastelfreak
Copy link
Member

Can you please rebase against our latest master? Your branch isn't up2date. Let me know if you need some help with git. You can reach us in #voxpupuli on freenode or on https://puppetcommunity.slack.com.

@@ -353,6 +358,9 @@
$agentx_socket = $snmp::params::agentx_socket,
$agentx_timeout = $snmp::params::agentx_timeout,
$agentx_retries = $snmp::params::agentx_retries,

Boolean $snmpv2_enable = $snmp::params::snmpv2_enable,
Copy link
Member

Choose a reason for hiding this comment

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

can you align the = with the others please?

@@ -404,6 +404,12 @@
$safe_trap_service_hasrestart = $trap_service_hasrestart
}

if $snmp_openmanage_enable {
$snmpv2_enable = $::snmp_snmpv2_enable
Copy link
Member

Choose a reason for hiding this comment

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

where is $snmp_snmpv2_enable coming from? Shouldn't we just do:

$snmpv2_enable =  false

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think you are right; This parameter come from init.pp and default value is in params.pp. I'll fix this.

@bastelfreak
Copy link
Member

@hdep can you take a look at the failing tests?

@hdep
Copy link
Contributor Author

hdep commented Sep 15, 2018

I guess default '$snmpv2_enable = false' broke the rspec test. I'll give a try with default to true.

@@ -1282,4 +1282,41 @@
end
end
end
context 'on a supported osfamily (Suse), with snmpv2_enable = true' do
Copy link
Member

Choose a reason for hiding this comment

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

can you please use rspec-puppet-facts here?

@hdep
Copy link
Contributor Author

hdep commented Sep 16, 2018

here it is. Currently '$snmpv2_enable = true'. If the rpsec is fine for you I can try to change the value to false.

@bastelfreak bastelfreak changed the title Disable snmpv2 Add $snmpv2_enable parameter Sep 19, 2018
@bastelfreak bastelfreak added enhancement New feature or request and removed needs-tests labels Sep 19, 2018
@bastelfreak
Copy link
Member

Thanks for the work @hdep! I squashed the commits and will merge it after travis is green.

@hdep
Copy link
Contributor Author

hdep commented Sep 21, 2018

Thank you for your help. Currently default will enable snmpv2, maybe later we can check to switch it to false later.

@bastelfreak bastelfreak merged commit ddde38f into voxpupuli:master Sep 23, 2018
@hdep hdep deleted the Disable-snmpv2 branch September 23, 2018 12:49
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