-
-
Notifications
You must be signed in to change notification settings - Fork 350
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
#49 Handle domain list change for a certificate #50
base: master
Are you sure you want to change the base?
Conversation
…ailed or is changed.
@MonsieurV Thanks! Can confirm this works very well. One thing about the persist-list: I think it would be better to name the file not by the first domain that is configured, cause there's a new file created when you put a new domain in the first position. |
@domhaas Thanks for the feedback. :) Do you have any suggestion for the file name? It should ideally be something derived from the domain lists (ensuring no collision if we have multiple certs) and as invariant as possible. Maybe we could add a variable for the persisted domain list filename (defaulting to the first domain). |
@geerlingguy I'm curious if there are any blockers to this being merged? It would be a nice touch to have this handled automatically. |
Any updates on this? |
This pull request has been marked 'stale' due to lack of recent activity. If there is no further activity, the PR will be closed in another 30 days. Thank you for your contribution! Please read this blog post to see the reasons why I mark pull requests as stale. |
I'm still using this solution and it does the job for me. |
This issue is no longer marked for closure. |
I'm on the fence on this; I don't generally like to pollute managed directories with files to track state, and that's what this seems to be doing (maintaining a list of domains in a file in the letsencrypt directories). |
Yes, that is what it does. I've hadn't found yet a way to compare this state (domains for a cert) between Ansible runs, without actually persisting it to the filesystem. Maybe |
This pull request has been marked 'stale' due to lack of recent activity. If there is no further activity, the PR will be closed in another 30 days. Thank you for your contribution! Please read this blog post to see the reasons why I mark pull requests as stale. |
This pull request is no longer marked for closure. |
I do want to fix this. I'm just giving it more time for consideration. I just hate adding state and complexity that this role introduces and then relies on. |
Either we add state, or we use the state already accessible. There is now a The ouput is like:
It seems doable to parse that, extract the certificate name (which matches the first domain used by this role for each domain list entry) and extract then compare the domain list. It won't be as easy to code as adding our own state file, but the state is here. Then we would need to run |
For solving #49, we persist to the host machine a file containing the list of domains for a given certificate.
If this list does not match the one provided in the current configuration, the certificate is issued again.
Beware: this make certificate expansion (
--expand
) the default implicit behavior. Maybe we could make it an option and only try to update the certificate if the variablecertbot_auto_expand
is set to true.Anyway, it now works as I would expect when using
certbot_auto_renew
: if we added a domain to one of our certificates, it is updated so the domain is now covered.