refactor: Use domains' setname as --cert-name option#76
Conversation
|
Best reviewed: commit by commit
Optimal code review plan
|
kiniou
force-pushed
the
use-domains-setname-as-cert-name-option
branch
from
8dc3102 to
c9b7db4
Compare
@kiniou Thanks for this PR. Hopefully, @javierbertoli will get a chance to review it soon. Don't worry about this just yet but if this PR is approved, we would require the commit to be amended so that the breaking change is applied by In terms of the content, I'm sure you could use pretty much what you've already provided in the section I've quoted. |
|
Hi @myii 👋 I've completely missed to read the CONTRIBUTING document and i would like to express my apologies. 😅 I'm actually using this change in my fork and i'm not in a hurry waiting for a proper review. 😉 Plus, this PR can be a place to discuss if we can transform this breaking change into some opt-in feature flag or by using a deprecation path otherwise. |
There was a problem hiding this comment.
Great addition, @kiniou, thanks!
As we're using semantic versioning, I'm perfectly OK with this PR changing the behavior (for good) of the formula and flag it.
myii
left a comment
myii
left a comment
There was a problem hiding this comment.
Thanks for the review, @javierbertoli.
Just using this Request changes as a placeholder to remind ourselves that the BREAKING CHANGE message needs to be added to the commit message before this is merged.
Certbot has the ability to name a certificate with the --cert-name option.
This allows us for instance to refers to this same setname to other
configuration and then further automate virtualhost creation in nginx or apache.
BREAKING CHANGE: Since this domains' setname was not used, the path was named
after the first domain in the domains set. In order to keep using this workflow,
you need to rename the setname with the first domain in the list like the
following:
```
letsencrypt:
domains:
foo.example.com:
- foo.example.com
```
kiniou
force-pushed
the
use-domains-setname-as-cert-name-option
branch
from
c9b7db4 to
68fb247
Compare
|
Hi @myii and @javierbertoli and thanks for the review 😊. I took a shoot at adding a BREAKING CHANGE paragraph. I'm just not sure if it will work with this kind of formatting (i.e code block). |
Let's merge it and see...! Thanks for the contribution, @kiniou! |
|
🎉 This PR is included in version 2.0.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
|
@kiniou The code block came through fine! |
4 participants
PR progress checklist (to be filled in by reviewers)
What type of PR is this?
Primary type
[build]Changes related to the build system[chore]Changes to the build process or auxiliary tools and libraries such as documentation generation[ci]Changes to the continuous integration configuration[feat]A new feature[fix]A bug fix[perf]A code change that improves performance[refactor]A code change that neither fixes a bug nor adds a feature[revert]A change used to revert a previous commit[style]Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc.)Secondary type
[docs]Documentation changes[test]Adding missing or correcting existing testsDoes this PR introduce a
BREAKING CHANGE?Yes.
The setname was not used before and the certificate path will not be the same.
The most obvious change that needs to be done is changing the pillar to match the first domain in a domain sets like the following:
Related issues and/or pull requests
Describe the changes you're proposing
Certbot has the ability to name a certificate with the --cert-name option.
This allows us for instance to refers to this same setname to other
configuration and then further automate virtualhost creation in nginx or apache.
Pillar / config required to test the proposed changes
Here is an example that should write the certificates into
/etc/letsencrypt/live/foo/path:You might need to change the
.example.comwith something real if tested against a real ACME server (I've tested this change locally with a local pebble server).Debug log showing how the proposed changes work
Documentation checklist
README(e.g.Available states).pillar.example.Testing checklist
state_top).Additional context