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

Fix path for crl_auto_renew with easy_rsa 3.0 #437

Merged
merged 1 commit into from
Jun 27, 2022
Merged

Fix path for crl_auto_renew with easy_rsa 3.0 #437

merged 1 commit into from
Jun 27, 2022

Conversation

cible
Copy link
Contributor

@cible cible commented Jun 21, 2022

Pull Request (PR) description

Small patch to fix the path of crl.pem introduced by #432 by @jkroepke
The cp fail with :

'./easyrsa gen-crl && cp ./keys/crl.pem /etc/openvpn/10.9.200.0 255.255.255.0/crl.pem' returned 1 instead of one of [0]
change from 'notrun' to ['0'] failed: './easyrsa gen-crl && cp ./keys/crl.pem /etc/openvpn/10.9.200.0 255.255.255.0/crl.pem' returned 1 instead of one of [0] (corrective)

Certainly a cut and paste problem. In manifests/revoke.pp, $server is the correct variable but not in server.pp

@root-expert root-expert added the bug Something isn't working label Jun 22, 2022
Copy link
Member

@root-expert root-expert left a comment

Choose a reason for hiding this comment

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

Thanks!

@jkroepke
Copy link
Contributor

Consider to add an test case for this. :)

@jkroepke
Copy link
Contributor

jkroepke commented Jun 22, 2022

@cible in fact you have to test if the puppet exec with command correct exists.

it { is_expected.to contain_exec('renew crl.pem on test_server') }

With exec resource itself is already including. The command parameter needs to be added, like (just an example):

it { is_expected.to contain_exec('renew crl.pem on test_server').with('command' => './easyrsa gen-crl && cp ./keys/crl.pem /etc/openvpn/server/crl.pem') } } 

In worse, an additional condition for easy-rsa 2.0 and 3.0 is needed

@cible
Copy link
Contributor Author

cible commented Jun 23, 2022

Sorry, I'm pretty bad with those kind of tests. Does it look good for you, now?

Copy link
Contributor

@jkroepke jkroepke left a comment

Choose a reason for hiding this comment

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

lgtm

@smortex
Copy link
Member

smortex commented Jun 26, 2022

Looks good! Can you please combine your 7 commits in a single one?

From your working directory:

git rebase -i 2b2168e796a58a851b13431e438a6fbf24b8d69f # Interactively rewrite history

This will bring your editor listing all your commits. Keep the first line unchanged, and replace pick with squash for all subsequent lines. Save and quit, your editor will pop again and allow you to combine all the commit messages in a single one.

Then push your updated branch:

git push -f            # Send the changes (-f is required because we re-wrote history)

Add test for for crl.pem with easyrsa 2.0 & 3.0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants