Skip to content
This repository has been archived by the owner on Aug 26, 2021. It is now read-only.

Reintroduce default ingress provider #257

Merged
merged 2 commits into from
Nov 7, 2017
Merged

Conversation

jackhopner
Copy link
Contributor

This implementation should be backwards compatible as
it will default the provider to the same value as the class.

Relates to #233

This implementation should be backwards compatible as
it will default the provider to the same value as the class
Copy link

@rowleyaj rowleyaj left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @jackhopner 👍

README.md Outdated
@@ -113,6 +113,7 @@ Please note:
| `LEGO_CHECK_INTERVAL` | n | `8h` | Interval for periodically certificate checks (to find expired certs)|
| `LEGO_MINIMUM_VALIDITY` | n | `720h` (30 days) | Request a renewal when the remaining certificate validity falls below that value|
| `LEGO_DEFAULT_INGRESS_CLASS` | n | `nginx` | Default ingress class for resources without specification|
| `LEGO_DEFAULT_INGRESS_PROVIDER` | n | `$LEGO_DEFAULT_INGRESS_CLASS` | Default ingress provider for resources without specification|

Choose a reason for hiding this comment

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

Could use a space before the last |. I know some of the lines above doesn't follow it, but the rest of the table seems to do so.

@munnerz
Copy link
Contributor

munnerz commented Nov 7, 2017

This LGTM. I think I removed code similar (although minus the defaulting to class) a while back. Thanks for working to get the default provider back in there without the backwards incompatibility!

@munnerz munnerz merged commit 039ab9c into jetstack:master Nov 7, 2017
@munnerz
Copy link
Contributor

munnerz commented Nov 7, 2017

& thanks very much for the contribution too..!

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

Successfully merging this pull request may close these issues.

4 participants