add spec tests, update validations, cleanup #31
Conversation
| if ( $equals == false ) and ( $pathname == '' ) { | ||
| fail('pathname must not be empty') | ||
| if !defined(Class['selinux']) { | ||
| fail('You must include the selinux base class before using any selinux defined resources') |
There was a problem hiding this comment.
Why fail? Can the base class just not be automatically imported for them?
There was a problem hiding this comment.
That is a possibility, but I've always preferred making it explicit what modules were going to do. Many of the resources required selinux::package which would also fail with an undefined resource, this just makes it a bit more explicit.
There was a problem hiding this comment.
If the base class was heavier, I'd agree. As it stands, the user really only is specifying the mode selinux is going to be in, so it's not too much of a stretch to provide a sane default and attempt to declare the class if it hasn't been before.
|
All in all, this is very nice and much needed. Thank you very much for adding. A few comments we're going back and forth on... my guiding factor is to try and make modules super user-friendly which is the backing behind a few of my comments. Sane defaults and non blocking behavior are preferred in my book. |
|
I updated the requirement that the selinux class is included outside the defines explicitly (just includes them now). Regarding wrapping things in warnings (after thinking about it for a while and talking with some other puppet guys), unless you don't want to use any stdlib validators (or puppet 4 typing) I think errors are the way to go. If you write all of the validators yourself you can achieve a consistent validations with warnings, but leveraging any stdlib validators will fail. Having some configurations that error (stdlib validators) and some that just warn (custom validators) will result in inconsistent feedback to the admin, but both of them will fail to apply catalogs as expected. |
|
I see you enabled travis too - nice! |
Ok, then let's punt on this for now. I still may revisit this, but time is not on my side, and these changes are 👍, so let's roll. |
add spec tests, update validations, cleanup
This was intended to just add spec tests, but writing spec tests highlighted some cleanup within the classes I thought needed to be done. Sorry for the size.
Enable travis-ci support
Add spec tests
Use validators from stdlib
Move OS detection logic to params class
Ensure selinux class is included before defines
Ensure helper classes cannot be called directly