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

Cleanly exit if cepces or getcert are not present #762

Merged
merged 4 commits into from
Aug 9, 2023

Conversation

GabrielNagy
Copy link
Contributor

In case certmonger or the cepces library are not installed we do not want to fail hard. Check for the existence of the binaries instead of debs to be more future-proof given that cepces is not yet packaged in Ubuntu.

Fixes UDENG-1156


The PR contains some logging improvements as well, and a small optimization to avoid creating the needed directories if the required binaries are not present.

Useful information like Samba logging is printed to stderr. We would
like to have this information even in the successful runs of the script.

It also adds value to print both stdout and stderr even in the failing
case.
@GabrielNagy GabrielNagy force-pushed the certificate-check-for-binaries branch from 5eead7f to ab81b95 Compare August 8, 2023 19:11
@GabrielNagy GabrielNagy marked this pull request as ready for review August 8, 2023 19:20
@GabrielNagy GabrielNagy requested a review from a team as a code owner August 8, 2023 19:20
@codecov-commenter
Copy link

Codecov Report

Merging #762 (ab81b95) into main (ced79b0) will increase coverage by 0.01%.
Report is 8 commits behind head on main.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #762      +/-   ##
==========================================
+ Coverage   86.05%   86.07%   +0.01%     
==========================================
  Files          77       77              
  Lines        8543     8552       +9     
==========================================
+ Hits         7352     7361       +9     
  Misses        868      868              
  Partials      323      323              
Files Changed Coverage Δ
internal/policies/certificate/cert-autoenroll 100.00% <100.00%> (ø)
internal/policies/certificate/certificate.go 95.65% <100.00%> (-0.07%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Member

@didrocks didrocks left a comment

Choose a reason for hiding this comment

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

No remark to add. I think we should think when writing the doc to mention how to install them, but otherwise, it looks really great!

We can leverage Samba's logging system to get a better understanding as
to what's going on during the lifetime of the autoenroll script.

Mock has to be wrangled a bit to satisfy the actual `lp` object.
In case certmonger or the cepces library are not installed we do not
want to fail hard. Check for the existence of the binaries instead of
debs to be more future-proof given that cepces is not yet packaged in
Ubuntu.

Fixes UDENG-1156
In case the binaries needed for certificate autoenrollment are not
present on the system, avoid creating the needed directories.

Also add a test to illustrate that we will not fail if the directory
structure is wonky, if certmonger is not present.
@GabrielNagy GabrielNagy force-pushed the certificate-check-for-binaries branch from ab81b95 to 845ccfe Compare August 9, 2023 09:27
@GabrielNagy GabrielNagy merged commit cc312eb into main Aug 9, 2023
4 checks passed
@GabrielNagy GabrielNagy deleted the certificate-check-for-binaries branch August 9, 2023 11:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants