feat(helm): allow extraArgs to also be a map enabling overrides of individual values

[frittentheke]

Description

With external-dns relying a lot on extraArgs for its configuration using a map (key: value) data structure for extraArgs enables a lot more versatility. Multiple values files can be used to e.g. configure common values in one and then only specific values in another.

Also setting certain values via --set is then possible - e.g to only change a single setting per environment or inject some value dynamically from CI.

Checklist

• [?] Unit tests updated

I did update the schema file

• [?] End user documentation updated

Maybe it makes sense to change the documented value type of extraArgs to map?

[k8s-ci-robot]

Hi @frittentheke. 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.

[stevehipwell]

@frittentheke what about moving the provider specific args into the provider variable as has already been done for webhook. For example.

provider:
  name: aws

  aws:
    customArg: good-default
    otherCustomArg:

This would be a better pattern for the common use cases as the inputs could be defined in the schema.

If you do want to update the extraArgs implementation, it probably needs to support a map of strings or string lists as idiomatically some args can be repeated.

extraArgs:
  my-arg: test
  my-other-arg:
    - foo
    - bar

[frittentheke]

@frittentheke what about moving the provider specific args into the provider variable as has already been done for webhook. For example.

provider:
  name: aws

  aws:
    customArg: good-default
    otherCustomArg:

This would be a better pattern for the common use cases as the inputs could be defined in the schema.

Yes. I am wondering if this would need to happen for all built-in providers in one PR to not make this more complex then necessary (by having e.g. AWS have all its options explicitly defined and others still only via extraArgs?

If you do want to update the extraArgs implementation, it probably needs to support a map of strings or string lists as idiomatically some args can be repeated.

extraArgs:
  my-arg: test
  my-other-arg:
    - foo
    - bar

Wow good catch, thank you @stevehipwell
I'll push this change soon. Maybe extraArgs to also potentially be a map is a good idea no matter if the provider specific configuration is moved to its own values / data structure.

[stevehipwell]

Yes. I am wondering if this would need to happen for all built-in providers in one PR to not make this more complex then necessary (by having e.g. AWS have all its options explicitly defined and others still only via extraArgs?

I don't think so, we already document providers with configuration support in the chart README. But a provider being added should be fully implemented with a typed JSON schema otherwise it doesn't make much sense.

Wow good catch, thank you @stevehipwell
I'll push this change soon. Maybe extraArgs to also potentially be a map is a good idea no matter if the provider specific configuration is moved to its own values / data structure.

Yes I agree that adding map support makes sense, there are likely cases where providers don't have specific configuration or where extra args are needed too.

[ivankatliarchuk]

Any chance to cover this funcitonality with helm unit tests as well?

/ok-to-test

[frittentheke]

@stevehipwell I pushed a new revision, including a unit test which @ivankatliarchuk asked for.

PTAL

[ivankatliarchuk]

/lgtm

[ivankatliarchuk]

need to re-run linters

[ivankatliarchuk]

/label tide/merge-method-squash

[frittentheke]

New changes are detected. LGTM label has been removed.

Sorry @ivankatliarchuk I missed to run helm-docs again causing the CI lint stage to fail.
Should (fingers crossed) be good now.

[frittentheke]

/test pull-external-dns-unit-test

[ivankatliarchuk]

/lgtm

[stevehipwell]

This is looking good @frittentheke, just a couple of comments for you.

[stevehipwell]

As a convention in this chart we keep template directives at the same indentation level as their parent. Could you please follow this pattern here?

[frittentheke]

@stevehipwell so you mean no indentation of any of the loops and conditionals then?
I pushed a change - PTAL

[stevehipwell]

Thanks for the quick responses @frittentheke, the changes look good. Sorry for missing this previously but please could you add an entry to the chart CHANGELOG.

[frittentheke]

Thanks for the quick responses @frittentheke, the changes look good. Sorry for missing this previously but please could you add an entry to the chart CHANGELOG.

DONE @stevehipwell

[stevehipwell]

Thanks for your patience @frittentheke.

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ivankatliarchuk, 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 [stevehipwell]

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