Skip to content

Docs: ACME Overhaul #3421

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.

Sign up for GitHub

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

Jump to bottom
Merged
traefiker merged 3 commits into from
Jun 5, 2018
Merged

Docs: ACME Overhaul #3421

traefiker merged 3 commits into from
Jun 5, 2018

Conversation

@dargmuesli
Copy link
Contributor

@dargmuesli dargmuesli commented Jun 1, 2018

What does this PR do?

  • Corrects several stylistic inadequacies
  • Orders topics lexicographically
  • Merges the two provider tables into one
  • Fixes some links
  • Removes duplication in multiple occurrences
  • Improves explanation of verification methods and problems

Motivation

The obvious stylistic inadequacies in the ACME docs article.

More

  • Added/updated documentation

Additional Notes

Please have a glance over my updated descriptions of methods and problems and check if they are indeed correct.

@mmatur mmatur added this to the 1.6 milestone Jun 4, 2018
@dtomcej
Copy link
Contributor

dtomcej commented Jun 4, 2018

@dargmuesli Can you please rebase this PR off the 1.6 branch instead of master?

Thanks!

@dargmuesli dargmuesli changed the base branch from master to v1.6 June 4, 2018 15:20
@dargmuesli dargmuesli requested review from a team as code owners June 4, 2018 15:24
@dargmuesli
Copy link
Contributor Author

dargmuesli commented Jun 4, 2018

Sure, did so. I hope I solved the merge conflicts corretly ;)

@ldez
Copy link
Contributor

ldez commented Jun 4, 2018

@dargmuesli could you rebase instead merge: git rebase --onto upstream/v1.6 <parent_commit>

@dargmuesli
Copy link
Contributor Author

dargmuesli commented Jun 4, 2018

Oh, sorry. I just followed GitHub's UI there. I guess I can force-push a rebased version?

@ldez ldez changed the base branch from v1.6 to master June 4, 2018 15:32
@ldez
Copy link
Contributor

ldez commented Jun 4, 2018 •
edited
Loading

I reseted your branch.
Now you can rebase git rebase --onto upstream/v1.6 HEAD^
And push force.

@dargmuesli
Copy link
Contributor Author

dargmuesli commented Jun 4, 2018

Sorry once again, I was just really confused which branch did what... I hope everything's correct now?

@ldez ldez changed the base branch from master to v1.6 June 4, 2018 15:54
@ldez ldez removed the bot/no-merge label Jun 4, 2018
dtomcej
dtomcej suggested changes Jun 4, 2018
View reviewed changes
docs/configuration/acme.md Outdated
Copy link
Contributor

@dtomcej dtomcej Jun 4, 2018

Choose a reason for hiding this comment

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

I personally prefer as well to too

Copy link
Contributor Author

@dargmuesli dargmuesli Jun 4, 2018

Choose a reason for hiding this comment

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

Good idea :)

docs/configuration/acme.md Outdated
Copy link
Contributor

@dtomcej dtomcej Jun 4, 2018

Choose a reason for hiding this comment

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

leave commented to go to prod

docs/configuration/acme.md Outdated
Copy link
Contributor

@dtomcej dtomcej Jun 4, 2018

Choose a reason for hiding this comment

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

not sure management is needed here

Copy link
Contributor Author

@dargmuesli dargmuesli Jun 4, 2018

Choose a reason for hiding this comment

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

Yeah well, it's arguably unnecessary.

docs/configuration/acme.md Outdated
Copy link
Contributor

@dtomcej dtomcej Jun 4, 2018

Choose a reason for hiding this comment

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

If delayBeforeCheck is greater than zero, this check is delayed for the configured duration in seconds.

docs/configuration/acme.md Outdated
Copy link
Contributor

@dtomcej dtomcej Jun 4, 2018

Choose a reason for hiding this comment

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

same as above

docs/configuration/acme.md Outdated
Copy link
Contributor

@dtomcej dtomcej Jun 4, 2018

Choose a reason for hiding this comment

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

It is not possible to request a double wildcard certificate for a domain (for example ..local.com)

docs/configuration/acme.md Outdated
Copy link
Contributor

@dtomcej dtomcej Jun 4, 2018

Choose a reason for hiding this comment

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

it is not possible

docs/configuration/acme.md Outdated
Copy link
Contributor

@dtomcej dtomcej Jun 4, 2018

Choose a reason for hiding this comment

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

launched in a container, the storage

docs/configuration/acme.md Outdated
Copy link
Contributor

@dtomcej dtomcej Jun 4, 2018

Choose a reason for hiding this comment

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

Should this be 2. etc?

Copy link
Contributor Author

@dargmuesli dargmuesli Jun 4, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

This should be interpreted as 2. by markdown renderers (GitHub does that). This way one can easily add a new list element without the need to increment all following.

docs/configuration/acme.md Outdated
Copy link
Contributor

@dtomcej dtomcej Jun 4, 2018

Choose a reason for hiding this comment

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

which need Let's Encrypt certificates generated, the default

dtomcej
dtomcej approved these changes Jun 4, 2018
View reviewed changes
Copy link
Contributor

@dtomcej dtomcej left a comment

Choose a reason for hiding this comment

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

LGTM
:shipit:

@ldez ldez self-requested a review June 4, 2018 20:40
juliens
juliens approved these changes Jun 5, 2018
View reviewed changes
Copy link
Member

@juliens juliens left a comment

Choose a reason for hiding this comment

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

LGTM

ldez
ldez approved these changes Jun 5, 2018
View reviewed changes
Copy link
Contributor

@ldez ldez left a comment

Choose a reason for hiding this comment

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

LGTM

dargmuesli and others added 3 commits June 5, 2018 08:32
@dargmuesli @traefiker
Docs: ACME Overhaul
6e3cc4f 
~ Corrected several stylistic inadequacies
~ Ordered topics lexicographically
~ Merged the two provider tables into one
~ Fixed some links
~ Removed duplication in multiple occurrences
@dargmuesli @traefiker
Requested Changes
71fd111 
~ Implements requested changes by @dtomcej
@ldez @traefiker
review: minor changes.
0ae492d
@traefiker traefiker merged commit f317e50 into traefik:v1.6 Jun 5, 2018
@dargmuesli
Copy link
Contributor Author

dargmuesli commented Jun 5, 2018

Nice, thank you guys!

@dargmuesli dargmuesli deleted the docs_acme branch March 20, 2019 04:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@juliens juliens juliens approved these changes

+2 more reviewers

@ldez ldez ldez approved these changes

@dtomcej dtomcej dtomcej approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area/acme area/documentation kind/enhancement a new or improved feature. size/L

Projects

None yet

Milestone

1.6

Development

Successfully merging this pull request may close these issues.

7 participants

@dargmuesli @dtomcej @ldez @juliens @mmatur @nmengin @traefiker