Refactors (ready for review) #81

Phil-Friderici · 2023-08-18T13:44:07Z

PR to collect my little refactors:

move OS specific data into module hiera
move functionality from subclasses to main class
refactor unit tests to be stricter and more complete
other minor refactors

- be more complete and strict - utilize rspec-puppet-facts

kenyon

Lots of nice work here!

manifests/init.pp

kenyon · 2023-08-23T19:04:57Z

manifests/init.pp

+  String[1] $update_cmd                    = 'update-ca-trust extract',
+  String[1] $cert_dir_group                = 'root',
+  String[1] $cert_dir_mode                 = '0755',
+  Boolean $supported                       = false,


Not sure I like this supported parameter pattern. I don't think we do this in any of the Vox Pupuli modules. It seems redundant to just listing supported OSes in metadata.json. I suppose you are retaining existing behavior.

Yes this is only here to maintain the existing behaviour. Would be more than happy to remove it :)
Let @pcfens decide which route to go.

I do agree with @kenyon on this one. @pcfens doesn't seem to have any opinion?

manifests/init.pp

ashish1099 · 2023-12-07T17:31:13Z

Wow, would be nice to get this merged. so looking forward for this one

h-haaks · 2024-05-09T17:12:31Z

metadata.json

Kali was added to params.pp while this PR was up for review.
Should that be added to metadata as well?

h-haaks · 2024-05-09T17:14:12Z

manifests/init.pp

-    create_resources('ca_cert::ca', $ca_certs)
+  create_resources('ca_cert::ca', $ca_certs)
+
+  if $facts['os']['family'] == 'RedHat' and versioncmp($facts['os']['release']['full'], '7') < 0 {


No need for this as EL6 is long gone.

h-haaks · 2024-05-09T17:23:00Z

data/os/Debian.yaml

+ca_cert::trusted_cert_dir:      '/usr/local/share/ca-certificates'
+ca_cert::update_cmd:            'update-ca-certificates'
+ca_cert::cert_dir_group:        'staff'
+ca_cert::cert_dir_mode:         '2665'


Where does this default come from?
On a fresh install of ca-certificates in amd/debian:10,amd/debian:11 and amd/debian:12 containers the default is

root@60422e36a680:/# ls -l /usr/local/share/ total 0 drwxr-xr-x. 2 root root 6 May 9 14:06 ca-certificates

h-haaks · 2024-05-09T17:27:26Z

data/os/Debian.yaml

+ca_cert::cert_dir_mode:         '2665'
+ca_cert::package_name:          'ca-certificates'
+ca_cert::ca::ca_file_group:     'root'
+ca_cert::ca::ca_file_mode:      '0444'


Why not 0644 as default? Fresh debian container once more:
No files at all in /usr/local/share/ca-certificates and certificates in /usr/share/ca-certificates have 0644

root@b3059a7e2497:/# ls -l /usr/share/ca-certificates/mozilla/ total 560 -rw-r--r--. 1 root root 2772 Mar 11 2023 ACCVRAIZ1.crt -rw-r--r--. 1 root root 1972 Mar 11 2023 AC_RAIZ_FNMT-RCM.crt -rw-r--r--. 1 root root 904 Mar 11 2023 AC_RAIZ_FNMT-RCM_SERVIDORES_SEGUROS.crt

/usr/local/share/ca-certificates is the right place for certs installed by this module. See the FILES section of man update-ca-certificates.

I'd be fine with 0644 or 0444, but I think we often use 0444 as a way to show that the files are puppet-managed and shouldn't be manually edited.

I was just referring to /usr/share/ca-certificates because that's provided by the ca-certificates package with mode 0644 as an argument for using the same when adding files to /usr/local/share/ca-certificates :)

h-haaks · 2024-05-09T17:36:52Z

data/os/Ubuntu.yaml

+ca_cert::supported:             true
+ca_cert::trusted_cert_dir:      '/usr/local/share/ca-certificates'
+ca_cert::update_cmd:            'update-ca-certificates'
+ca_cert::cert_dir_group:        'staff'


Why staff as default? The ca-certificates package creates this directory with group root ...

root@cbe1f3cd0871:/# ls -l /usr/local/share/ total 0 drwxr-xr-x. 2 root root 6 May 9 17:31 ca-certificates

h-haaks · 2024-05-09T17:38:41Z

data/os/Ubuntu.yaml

+ca_cert::cert_dir_mode:         '0755'
+ca_cert::package_name:          'ca-certificates'
+ca_cert::ca::ca_file_group:     'root'
+ca_cert::ca::ca_file_mode:      '0444'


Same reasoning as for Debian. See no reason why this should default to 0444 ..

h-haaks · 2024-05-09T17:50:57Z

I guess changing the default for Debian family should be done in a separate PR so that it shows in changelog :)

h-haaks · 2024-05-09T20:22:30Z

This PR has a few conflicts and is changing things that should to be split into separate PRs.
I'll try to do this the best I can.
Hope that's all right with you @Phil-Friderici

Phil-Friderici · 2024-05-10T07:04:20Z

Yes sure modify as needed. As I’m not able to contribute anymore myself, I’m happy to see any action here :)This PR has a few conflicts and is changing things that should to be split into separate PRs. I'll try to do this the best I can. Hope that's all right with you @Phil-Friderici

h-haaks · 2024-05-29T14:56:17Z

@Phil-Friderici I think I have covered all the relevant parts of this PR now.
Could you please have a look at the current code in master and close this PR if you agree?

TheMeier · 2024-06-02T07:36:46Z

@h-haaks it needs a rebase though

h-haaks · 2024-06-03T11:59:32Z

@TheMeier I don't see why the PR should be rebased before closing ( no merge ).

h-haaks · 2024-06-05T19:13:07Z

I'll move on and close this as I think all the relevant pieces have been solved by other smaller PR's.
Please feel free to reopen or create new PRs if I missed anything.

Phil Friderici added 4 commits August 18, 2023 08:54

Refactor unit tests

654913a

- be more complete and strict - utilize rspec-puppet-facts

Testing: set resource coverage to 100%

8d1312b

Add supported OS families to metadata

08e52e0

Remove unnecessary code

843bbeb

Phil-Friderici force-pushed the refactors branch 2 times, most recently from c77c061 to 8f58fc1 Compare August 21, 2023 10:30

Move OS specific data from params.pp into hiera

7a2ab4a

Phil-Friderici force-pushed the refactors branch from 8f58fc1 to 7a2ab4a Compare August 22, 2023 10:53

Phil Friderici added 4 commits August 22, 2023 12:31

Move functionality of ca_cert::enable to ca_cert::update

083b731

Move functionality of ca_cert::update to main class

0049e4e

Minor refactors

183df36

Add description for new parameters

b7e15fc

Phil-Friderici force-pushed the refactors branch from 3c3288a to b7e15fc Compare August 23, 2023 10:13

Phil-Friderici changed the title ~~Refactors (work in progress)~~ Refactors (ready for review) Aug 23, 2023

kenyon reviewed Aug 23, 2023

View reviewed changes

Phil Friderici added 2 commits August 24, 2023 13:13

Remove unneeded parentheses

77d05d0

Convert inline docs to Puppet Strings

cddd3cf

h-haaks reviewed May 9, 2024

View reviewed changes

metadata.json

Copy link

Contributor

h-haaks May 9, 2024

Choose a reason for hiding this comment

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

Kali was added to params.pp while this PR was up for review.
Should that be added to metadata as well?

h-haaks reviewed May 9, 2024

View reviewed changes

h-haaks mentioned this pull request May 9, 2024

Add supported OSes #94

Merged

h-haaks mentioned this pull request May 14, 2024

Remove ca_cert::update class #98

Merged

h-haaks mentioned this pull request May 24, 2024

Move and use params only in ca_certs class #99

Merged

This was referenced May 27, 2024

Cleanup Debian defaults #101

Merged

Move OS specific data from params.pp into hiera #102

Merged

Refactor ca_cert:ca unit tests to use on_supported_os #105

Merged

h-haaks mentioned this pull request Jun 5, 2024

Refactor unit testing #80

Closed

h-haaks closed this Jun 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactors (ready for review) #81

Refactors (ready for review) #81

Phil-Friderici commented Aug 18, 2023 •

edited

Loading

kenyon left a comment

kenyon Aug 23, 2023

Phil-Friderici Aug 24, 2023

h-haaks May 9, 2024

ashish1099 commented Dec 7, 2023

h-haaks May 9, 2024

h-haaks May 9, 2024

h-haaks May 9, 2024

h-haaks May 9, 2024

kenyon May 9, 2024

h-haaks May 9, 2024

h-haaks May 9, 2024

h-haaks May 9, 2024

h-haaks commented May 9, 2024 •

edited

Loading

h-haaks commented May 9, 2024

Phil-Friderici commented May 10, 2024 via email

h-haaks commented May 29, 2024

TheMeier commented Jun 2, 2024

h-haaks commented Jun 3, 2024 •

edited

Loading

h-haaks commented Jun 5, 2024

Refactors (ready for review) #81

Refactors (ready for review) #81

Conversation

Phil-Friderici commented Aug 18, 2023 • edited Loading

kenyon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashish1099 commented Dec 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

h-haaks commented May 9, 2024 • edited Loading

h-haaks commented May 9, 2024

Phil-Friderici commented May 10, 2024 via email

h-haaks commented May 29, 2024

TheMeier commented Jun 2, 2024

h-haaks commented Jun 3, 2024 • edited Loading

h-haaks commented Jun 5, 2024

Phil-Friderici commented Aug 18, 2023 •

edited

Loading

h-haaks commented May 9, 2024 •

edited

Loading

h-haaks commented Jun 3, 2024 •

edited

Loading