extract nssdb creation into separate class #139
Conversation
timogoebel
force-pushed
the
extract_nssdb
branch
from
fee602a to
9e16f7a
Compare
manifests/ssltools/create_nssdb.pp
Outdated
| owner => 'root', | ||
| group => $::certs::qpidd_group, | ||
| mode => '0755', | ||
| } ~> |
There was a problem hiding this comment.
I think this doesn't have to be a notify
manifests/ssltools/create_nssdb.pp
Outdated
| owner => 'root', | ||
| group => $::certs::qpidd_group, | ||
| mode => '0640', | ||
| } ~> |
There was a problem hiding this comment.
I don't think this has to be a notify
manifests/ssltools/create_nssdb.pp
Outdated
| command => "certutil -N -d '${::certs::nss_db_dir}' -f '${nss_db_password_file}'", | ||
| path => '/usr/bin', | ||
| creates => $nssdb_files, | ||
| } ~> |
There was a problem hiding this comment.
I don't think this has to be a notify
manifests/ssltools/create_nssdb.pp
Outdated
| @@ -0,0 +1,35 @@ | |||
| # Creates a nssdb | |||
| class certs::ssltools::create_nssdb inherits certs { | |||
There was a problem hiding this comment.
Maybe just name it ```nssdb``? The current name sounds like a command.
There was a problem hiding this comment.
I'd also prefer if this was just a class with a parameter rather than inheriting from certs. Looks like there are just 2 (nss_db_dir and group).
timogoebel
force-pushed
the
extract_nssdb
branch
from
9e16f7a to
97be3f3
Compare
|
@ekohl : Rebased and addressed your comments. |
manifests/ssltools/nssdb.pp
Outdated
| group => $::certs::qpidd_group, | ||
| mode => '0755', | ||
| } -> | ||
| exec { 'generate-nss-password': |
There was a problem hiding this comment.
While we're at it, maybe set the umask as well for security reasons?
There was a problem hiding this comment.
Yes, and maybe if we add group => $group the file resource won't change anything at all (but still should be there to ensure it's correct).
manifests/ssltools/nssdb.pp
Outdated
| group => $::certs::qpidd_group, | ||
| mode => '0640', | ||
| } -> | ||
| exec { 'create-nss-db': |
manifests/ssltools/nssdb.pp
Outdated
| @@ -0,0 +1,35 @@ | |||
| # Creates a nssdb | |||
| class certs::ssltools::nssdb inherits certs { | |||
There was a problem hiding this comment.
Could you implement it as a non-inheriting class with parameters? That would make testing easier and faster.
There was a problem hiding this comment.
@ekohl: How do you suggest that we pass the parameters? That would mean we can't call the class with include but have to use class {'':} although we have to call the class several times?
There was a problem hiding this comment.
If they default to $::certs::pki_dir and $::certs__qpidd_group a simple include should work I think.
timogoebel
force-pushed
the
extract_nssdb
branch
3 times, most recently
from
84a8ba5 to
02ab6e7
Compare
|
@ekohl : The class does not inherit from certs anymore. Added the |
manifests/ssltools/nssdb.pp
Outdated
| # Sets up nssdb | ||
| class certs::ssltools::nssdb ( | ||
| $nss_db_dir = $::certs::nss_db_dir, | ||
| $qpidd_group = $::certs::qpidd_group |
There was a problem hiding this comment.
Given this is now just about creating the nss db I think it could even be just $group = ... and use a trailing comma.
timogoebel
force-pushed
the
extract_nssdb
branch
from
02ab6e7 to
8900a12
Compare
|
Updated. |
|
Now that the pipeline is back to green, I think this should be next to merge to unblock the other PRs. |
|
Thanks @timogoebel |
Both
qpiddandcandlepinneed thenssdbpresent. This commit extracts the code to a common class.