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

Add options labelFilter and managedRecordTypes in Helm Chart #4849

Merged
merged 3 commits into from
Nov 20, 2024
Merged

Add options labelFilter and managedRecordTypes in Helm Chart #4849

merged 3 commits into from
Nov 20, 2024

Conversation

abaguas
Copy link
Contributor

@abaguas abaguas commented Nov 5, 2024 •
edited
Loading

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

  • Unit tests updated
  • End user documentation updated

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 5, 2024
@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Nov 5, 2024
@k8s-ci-robot
Copy link
Contributor

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. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Nov 5, 2024
@abaguas
Add options to configure labelFilter and managedRecordTypes in He…
0eab887 
…lm Chart

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

In the [K8GB project](https://github.com/k8gb-io/k8gb), 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.
@abaguas abaguas force-pushed the helmchart/addoptions branch from 40ee980 to 0eab887 Compare November 5, 2024 07:42
stevehipwell
stevehipwell suggested changes Nov 18, 2024
View reviewed changes
Copy link
Contributor

@stevehipwell stevehipwell left a comment

Choose a reason for hiding this comment

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

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.

charts/external-dns/CHANGELOG.md Outdated Show resolved Hide resolved
@abaguas
update changelog & ci-values
c20fdea 
Signed-off-by: Andre Aguas <[email protected]>
@abaguas
Copy link
Contributor Author

abaguas commented Nov 18, 2024 •
edited
Loading

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.

stevehipwell
stevehipwell suggested changes Nov 20, 2024
View reviewed changes
Copy link
Contributor

@stevehipwell stevehipwell left a comment

Choose a reason for hiding this comment

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

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.

charts/external-dns/values.yaml Outdated
@@ -229,6 +229,12 @@ domainFilters: []
## -- Intentionally exclude domains from being managed.
excludeDomains: []

## -- (string) Filter resources queried for endpoints by label selector
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
## -- (string) Filter resources queried for endpoints by label selector
# -- (string) Filter resources queried for endpoints by label selector

Could you also fix the 2 values above to use a # -- prefix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see where the mistake came from, thanks. I fix mine and the ones above. The descriptions are now generated

charts/external-dns/values.yaml Outdated Show resolved Hide resolved
@abaguas
Copy link
Contributor Author

abaguas commented Nov 20, 2024

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
Copy link
Contributor

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 20, 2024
@stevehipwell
Copy link
Contributor

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 20, 2024
@k8s-ci-robot
Copy link
Contributor

[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:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 20, 2024
@k8s-ci-robot k8s-ci-robot merged commit 850b973 into kubernetes-sigs:master Nov 20, 2024
15 checks passed
@lucasfcnunes
Copy link

@abaguas

I was thinking I was using --label-filter in extraArgs and was going to change it... but I'm using --annotation-filter 😆

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stevehipwell stevehipwell stevehipwell requested changes

@mloiseleur mloiseleur Awaiting requested review from mloiseleur

Assignees

@stevehipwell stevehipwell

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@abaguas @k8s-ci-robot @stevehipwell @lucasfcnunes