kubernetes-sigs / external-dns

Configure external DNS servers (AWS Route53, Google CloudDNS and others) for Kubernetes Ingresses and Services
Apache License 2.0
7.76k stars 2.58k forks source link

Add options `labelFilter` and `managedRecordTypes` in Helm Chart #4849

Closed abaguas closed 1 week ago

abaguas commented 3 weeks ago

Description

By promoting this options to dedicated values they no longer have to be configured via extraArgs.

Motivation

In the K8GB project, a DNS based load balancer, we use external-dns as a chart dependency. We would like to configure all values specific to the controller in the default values of our Chart, and leave to the users the provider configuration. This provider configuration usually includes extraArgs. Since extraArgs is a list that would be overwritten we would like to keep it empty, otherwise users will have to copy paste our base configuration.

Checklist

k8s-ci-robot commented 3 weeks ago

Hi @abaguas. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.
abaguas commented 1 week ago

Thanks for the PR @abaguas, this make sense given the fact that the args are generic.

Could you please run helm-docs locally to update the README for the new values.

Could you also update charts/external-dns/ci/ci-values.yaml to use the new values you've added. Records could just be setting the implicit defaults explicitly and the selector could be a wildcard.

Thank you for the review. Done, but I have a couple of doubts.

What is the role of the charts/external-dns/ci/ci-values.yaml file? It was last updated 1 year ago and I didn't find any code references to it? https://github.com/kubernetes-sigs/external-dns/commits/master/charts/external-dns/ci

What is a wildcard selector? The selector is a string of the form <key>=<value>. A * would be taken literally which is not what we want. What am I missing?

The README was already updated. Running helm-docs resulted in no changes.

abaguas commented 1 week ago

What is the role of the charts/external-dns/ci/ci-values.yaml file? It was last updated 1 year ago and I didn't find any code references to it? https://github.com/kubernetes-sigs/external-dns/commits/master/charts/external-dns/ci

Helm chart testing uses this to set the chart values.

What is a wildcard selector? The selector is a string of the form =. A * would be taken literally which is not what we want. What am I missing?

You're right. I think for testing purposes it just needs to be a valid selector.

The README was already updated. Running helm-docs resulted in no changes.

The syntax is incorrect which is why the README descriptions aren't being populated. I've suggested the fixes including the existing ones which look to be what you copied.

Thank you for the feedback. I added a valid selector for helm testing and fixed the helm comments 👍

stevehipwell commented 1 week ago

/ok-to-test

stevehipwell commented 1 week ago

/lgtm /approve

k8s-ci-robot commented 1 week ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: stevehipwell

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files: - ~~[charts/OWNERS](https://github.com/kubernetes-sigs/external-dns/blob/master/charts/OWNERS)~~ [stevehipwell] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment