-
Notifications
You must be signed in to change notification settings - Fork 101
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
NS1 support #217
NS1 support #217
Conversation
ytsarev
commented
Dec 13, 2020
•
edited
Loading
edited
- Chart extension for external-dns based ns1 provider
- Move common code between route53 and NS1(and potentially other providers) to separate function
- Depresolver extension with NS1 provider type
- Depresolver and controller tests
- Chart version bump
LGTM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor suggestions
Co-authored-by: Donovan Muller <[email protected]>
Co-authored-by: Donovan Muller <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
two open questions from my side, otherwise looks great!
@@ -86,3 +86,7 @@ spec: | |||
- name: ROUTE53_ENABLED | |||
value: "true" | |||
{{ end }} | |||
{{ if .Values.ns1.enabled }} | |||
- name: NS1_ENABLED | |||
value: "true" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does helm support something like this ?
- name: ROUTE53_ENABLED
value: {{ .Values.route53.enabled }}
- name: NS1_ENABLED
value: {{ .Values.ns1.enabled }}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kuritka maybe but it will be declared but empty variables, do not think we want to pollute the environment with them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm