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.

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

Pass endpoint params for ns1 external-dns provider #470

Merged
merged 1 commit into from
May 6, 2021
Merged

Pass endpoint params for ns1 external-dns provider #470

merged 1 commit into from
May 6, 2021

Conversation

k0da
Copy link
Collaborator

@k0da k0da commented May 6, 2021

Signed-off-by: Dinar Valeev [email protected]

ytsarev
ytsarev requested changes May 6, 2021
View reviewed changes
docs/examples/ns1/k8gb-cluster-ns1-us-east-1.yaml Outdated
@@ -8,3 +8,5 @@ k8gb:

ns1:
enabled: true
# endpoint: https://ns1.example.com
# ignoreSSL: false
Copy link
Member

Choose a reason for hiding this comment

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

ignoreSSL can stay false uncommented

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This way it will be set into deployment, which makes args list longer and hard to read. I'd leave defaults into provider's default settings

Copy link
Member

Choose a reason for hiding this comment

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

No, in case of false the if in the template will not add the parameter at all

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@ytsarev amended

Copy link
Member

Choose a reason for hiding this comment

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

@k0da but apparently not pushed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry, missed that you referenced example values, while I've been amending default chart values. Fixed now

chart/k8gb/values.yaml Outdated
@@ -62,3 +62,5 @@ route53:

ns1:
enabled: false
# endpoint: http://ns1.example.com
Copy link
Member

Choose a reason for hiding this comment

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

please mentioned in the comment that the value is optional and it defaults to managed NS1

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@ytsarev done

@k0da k0da force-pushed the ns1 branch 2 times, most recently from 14bccab to de2e682 Compare May 6, 2021 08:03
@somaritane
Copy link
Contributor

somaritane commented May 6, 2021 •
edited
Loading

@k0da , @ytsarev, do we want to also propagate MinTTLSeconds? Or are we OK with default TTL?

ytsarev
ytsarev requested changes May 6, 2021
View reviewed changes
docs/examples/ns1/k8gb-cluster-ns1-us-east-1.yaml Outdated
@@ -8,3 +8,5 @@ k8gb:

ns1:
enabled: true
# endpoint: https://ns1.example.com
# ignoreSSL: false
Copy link
Member

Choose a reason for hiding this comment

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

@k0da but apparently not pushed?

@k0da
Copy link
Collaborator Author

k0da commented May 6, 2021

@k0da , @ytsarev, do we want to also propagate MinTTLSeconds? Or are we OK with default TTL?

Do we have need for that atm?

@ytsarev
Copy link
Member

ytsarev commented May 6, 2021

@k0da , @ytsarev, do we want to also propagate MinTTLSeconds? Or are we OK with default TTL?

Do we have need for that atm?

depends on the default. If it is higher than our 30sec we might need to override it. Overall, the collision with DNSEndpoint CR does not look convenient. I am not sure about behavior here at all btw - will minTTL override specific CR TTL value? Does it have precedence? We need to figure out. Anyway, it does not block this specific PR

ytsarev
ytsarev reviewed May 6, 2021
View reviewed changes
docs/examples/ns1/k8gb-cluster-ns1-eu-west-1.yaml Outdated
@@ -8,3 +8,5 @@ k8gb:

ns1:
enabled: true
# endpoint: https://ns1.example.com
Copy link
Member

Choose a reason for hiding this comment

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

why do we need it in this sample?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

same as below

docs/examples/ns1/k8gb-cluster-ns1-us-east-1.yaml Outdated
@@ -8,3 +8,5 @@ k8gb:

ns1:
enabled: true
# endpoint: https://ns1.example.com
Copy link
Member

Choose a reason for hiding this comment

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

same as above

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

dropped

somaritane
somaritane suggested changes May 6, 2021
View reviewed changes
somaritane
somaritane approved these changes May 6, 2021
View reviewed changes
Copy link
Contributor

@somaritane somaritane left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@k0da k0da merged commit afc60ee into master May 6, 2021
@k0da k0da deleted the ns1 branch May 6, 2021 08:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@somaritane somaritane somaritane approved these changes

@ytsarev ytsarev ytsarev approved these changes

@donovanmuller donovanmuller Awaiting requested review from donovanmuller donovanmuller is a code owner

@kuritka kuritka Awaiting requested review from kuritka kuritka is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@k0da @somaritane @ytsarev