Skip to content

fix snmptrapd on ubuntu and debian #168

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
Merged
merged 2 commits into from
Nov 23, 2018
Merged

fix snmptrapd on ubuntu and debian #168

merged 2 commits into from
Nov 23, 2018

Conversation

@amateo
Copy link
Contributor

@amateo amateo commented Nov 12, 2018

Since Ubuntu 16.04 snmptrad is provided by another package (snmptrapd).

@amateo amateo changed the title Install snmptrapd on latests ubuntu [WIP] Install snmptrapd on latests ubuntu Nov 12, 2018
@amateo amateo changed the title [WIP] Install snmptrapd on latests ubuntu Install snmptrapd on latests ubuntu Nov 20, 2018
@amateo
Copy link
Contributor Author

amateo commented Nov 20, 2018

I think I have just fixed all test problems.

@Dan33l
Copy link
Member

Dan33l commented Nov 20, 2018

Hi @amateo thank you for this PR.

Is it duplicate with old #114 ? If yes can you close the old #114.

Currently the module does not have acceptance about snmptrapd. Can you add beaker acceptance tests about this ?

We can gives some guidance about acceptance tests with beaker.
The current acceptance tests are here :
https://github.com/voxpupuli/puppet-snmp/blob/master/spec/acceptance/snmp_spec.rb

Dan33l
Dan33l reviewed Nov 20, 2018
View reviewed changes
manifests/init.pp Outdated
require => Package['snmpd'],
notify => Service['snmptrapd'],
}
} elsif $facts['os']['family'] == 'Debian' and versioncmp($facts['os']['release']['major'], '16.04') >= 0 {
Copy link
Member

@Dan33l Dan33l Nov 20, 2018

Choose a reason for hiding this comment

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

\o/ structred facts . I am happy.

Dan33l
Dan33l reviewed Nov 20, 2018
View reviewed changes
spec/classes/snmp_init_spec.rb
).that_requires('Package[snmpd]').that_notifies('Service[snmpd]')
}
it { is_expected.not_to contain_file('snmptrapd.sysconfig') }
case facts[:os]['release']['major']
Copy link
Member

@Dan33l Dan33l Nov 20, 2018

Choose a reason for hiding this comment

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

yeeess structured facts \o/

Dan33l
Dan33l requested changes Nov 20, 2018
View reviewed changes
@amateo amateo mentioned this pull request Nov 21, 2018
Closed
@amateo
Copy link
Contributor Author

amateo commented Nov 21, 2018

Is it duplicate with old #114 ? If yes can you close the old #114.

Yes, it is. At some point, I had to rebase with latest changes in this repo. I have just closed the #114.

Dan33l
Dan33l reviewed Nov 21, 2018
View reviewed changes
Copy link
Member

@Dan33l Dan33l left a comment

Choose a reason for hiding this comment

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

I set this previous question as a review comment :

Currently the module does not have acceptance about snmptrapd. Can you add beaker acceptance tests about this ?

We can gives some guidance about acceptance tests with beaker.
The current acceptance tests are here :
https://github.com/voxpupuli/puppet-snmp/blob/master/spec/acceptance/snmp_spec.rb

@amateo
Copy link
Contributor Author

amateo commented Nov 21, 2018 •
edited
Loading

@Dan33l , sorry, I missed it with the other comments.

Can you guide to me? What is the purpose of this acceptance test?

I can add:

describe process('snmptrapd') do
  it { is_expected.not_to be_running }
end

(with the provided manifests it shouldn't be running).

But I'm not really sure it this is enough.

Or should I provide another configuration where snmptrapd should be running?

@Dan33l
Copy link
Member

Dan33l commented Nov 21, 2018

It looks that parameter trap_service_ensure defaults to stopped.
Currently the class is called without trap_service_ensure and so default value is used.

First step should be to play around this parameter.
And then probably add something around the test you proposed.

@vStone
Copy link
Contributor

vStone commented Nov 22, 2018

Most of this is also in #156

@amateo
Copy link
Contributor Author

amateo commented Nov 22, 2018

It looks that parameter trap_service_ensure defaults to stopped.
Currently the class is called without trap_service_ensure and so default value is used.

First step should be to play around this parameter.
And then probably add something around the test you proposed.

I have configured the 'with snmpv3 parameters context so, as no trap_service_ensure is provided, it checks that snmptrapd is not running.

Then I have another context to check that when trap_service_ensure is provided, the service should be running.

If you think there should be done different/more tests, tell me.

Dan33l
Dan33l requested changes Nov 22, 2018
View reviewed changes
Copy link
Member

@Dan33l Dan33l left a comment

Choose a reason for hiding this comment

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

When odd alignments are fixed, the PR looks to be ready for merge.

spec/acceptance/snmp_spec.rb Outdated
authpass => '456authpass',
privpass => '789privpass',
}
privpass => '789privpass',
Copy link
Member

@Dan33l Dan33l Nov 22, 2018

Choose a reason for hiding this comment

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

This pp part does not have to be modified.
Odd alignments have to be fixed.

@amateo
Install snmptrapd on latests ubuntu
e30d14f 
Since Ubuntu 16.04 snmptrad is provided by another package (snmptrapd).
@amateo amateo mentioned this pull request Nov 23, 2018
Merged
Dan33l
Dan33l requested changes Nov 23, 2018
View reviewed changes
Copy link
Member

@Dan33l Dan33l left a comment

Choose a reason for hiding this comment

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

Test "snmp class with snmptrapd running Process snmptrapd should be running" is now failing. Can you have a look ?

@amateo
Copy link
Contributor Author

amateo commented Nov 23, 2018

Test "snmp class with snmptrapd running Process snmptrapd should be running" is now failing. Can you have a look ?

Sorry, I deleted a commit fixing indentation. It should be fixed now

@amateo
Copy link
Contributor Author

amateo commented Nov 23, 2018

I has failed in puppet6 on debian 8 test, but the fail is not directly related with puppet code, but connecting to the docker container (I think).

@amateo
Configures snmptrapd in debian 8 and 9
2881774 
They both use a snmptrapd package too
@amateo
Copy link
Contributor Author

amateo commented Nov 23, 2018

All fixed.

@Dan33l
Copy link
Member

Dan33l commented Nov 23, 2018

LGTM. Thank you for this PR.

@Dan33l Dan33l merged commit 021c1b2 into voxpupuli:master Nov 23, 2018
@Dan33l Dan33l added this to the 4.1.0 milestone Nov 23, 2018
@Dan33l Dan33l added the bug Something isn't working label Nov 23, 2018
@Dan33l Dan33l changed the title Install snmptrapd on latests ubuntu fix snmptrapd on ubuntu and debian Nov 23, 2018
@amateo amateo deleted the snmptrapd branch November 26, 2018 09:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alexjfisher alexjfisher alexjfisher left review comments

@Dan33l Dan33l Dan33l approved these changes

Assignees

No one assigned

Labels

bug Something isn't working

Projects

None yet

Milestone

4.1.0

Development

Successfully merging this pull request may close these issues.

4 participants

@amateo @Dan33l @vStone @alexjfisher