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

Add "--expand" option for Certbot #171

Closed
wants to merge 3 commits into from
Closed

Conversation

amotl
Copy link

@amotl amotl commented Jan 24, 2022

Dear Jeff,

we want to salute you once more for one of your excellent contributions to the Ansible community in form of this recipe.

Because we want to aim at maximum DWIM by implementing our systems automation infrastructure with Ansible, we are in dire need of support for the --expand option of Certbot, using its webroot method. So, we picked up the contribution #117 by @ymarkus (thanks a stack!), wrapped it up and added corresponding support for webroot.

After that, expanding the list of certbot_certs.domains by another item and re-running the corresponding playbook immediately resolved the problem for us, where, beforehand, another subdomain was added to the list and the recipe was not able to pick up the change, without reporting back any kind of error.

We hope you will like the patch. I will add some comments to the PR, where I believe details about the implementation should be discussed. Thank you for taking the time to look into this!

With kind regards,
Andreas.


Others also needing this: In order to install the improvements in this branch into your Ansible environment, you might either want to invoke

ansible-galaxy install git+https://github.com/cicerops/ansible-role-certbot.git,expand

or add this to your requirements.yaml file:

roles:

  - name: geerlingguy.certbot
    src: git+https://github.com/cicerops/ansible-role-certbot
    version: expand

@@ -35,7 +35,7 @@ certbot_create_command: >-
--email {{ cert_item.email | default(certbot_admin_email) }}
{{ '--webroot-path ' if certbot_create_method == 'webroot' else '' }}
{{ cert_item.webroot | default(certbot_webroot) if certbot_create_method == 'webroot' else '' }}
-d {{ cert_item.domains | join(',') }}
--domains {{ cert_item.domains | join(',') }} --expand
Copy link
Author

Choose a reason for hiding this comment

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

Do you believe the --expand option should be wrapped into another flag which determines its presence?

We are so used to using it all across the board, that we wouldn't even bother to omit it ;]. Do you see any negative impact on just having it as a default?

---
- name: Get installed certificates.
shell: |
{{ certbot_script }} certificates | grep "Domains:" | awk '{ gsub(/ Domains: /,""); print }'
Copy link
Author

Choose a reason for hiding this comment

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

This might only work on Linux. macOS, by default, ships with corresponding non-GNU programs, where some of them have a different behavior. It will also not work natively on Windows.

So, I believe this spot should be improved?

Copy link
Author

Choose a reason for hiding this comment

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

Hi again. 9c0d000 improves the situation by removing any bashisms.

@stale
Copy link

stale bot commented Apr 25, 2022

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.

@stale stale bot added the stale label Apr 25, 2022
@stale
Copy link

stale bot commented Jun 4, 2022

This pull request has been closed due to inactivity. If you feel this is in error, please reopen the pull request or file a new PR with the relevant details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants