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

Improve installation of certificate #7

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

wsmirnow
Copy link
Contributor

@wsmirnow wsmirnow commented Jan 9, 2025

This task make no sense without nginx. But the ssl directory may not exist. This role should not fail in this case.

@wsmirnow wsmirnow force-pushed the improve-cert-installation branch from fb6dffc to 61c9791 Compare January 10, 2025 08:48
@wsmirnow
Copy link
Contributor Author

The linting errors are fixed by #6

Copy link
Collaborator

@lkiesow lkiesow left a comment

Choose a reason for hiding this comment

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

This looks reasonable.
It seems like the CI errors stem from older problems and should hopefully disappear when you rebase this. If that's the case, feel free to merge this on your own.

Comment on lines +59 to +62
- name: Reload nginx
ansible.builtin.systemd:
name: nginx
state: reloaded
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not necessarily something to change, just a question for my understanding:
Is this to ensure we already have valid certificates loaded for later roles?
Because otherwise, we could work with a notify:.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it is. The nginx reload will be run always. This is ok, because we do not change nginx configuration but only replace the certificate. We assume the nginx config is valid as the daemon is running. Otherwise the ACME webroot challenge will not succeed.

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.

2 participants