helm: Replaced object-based extraArgs with array-based

[sergeimonakhov]

Signed-off-by: Sergey Monakhov monakhov@puzl.ee

Please ensure your pull request adheres to the following guidelines:

• For first time contributors, read Submitting a pull request
• All code is covered by unit and/or runtime tests where feasible.
• All commits contain a well written commit description including a title,
  description and a Fixes: #XXX line if the commit addresses a particular
  GitHub issue.
• All commits are signed off. See the section Developer’s Certificate of Origin
• Provide a title or release-note blurb suitable for the release notes.
• Thanks for contributing!

Hi, I refactored extra-args.

Fixes: #15150

Replaced object-based extraArgs with array-based in Helm chart for all sub-components

[kkourt]

I remember that there was a discussion about this, so I found this comment that resulted in: #15150.

[kkourt]

Thanks!

[nbusseneau]

Thanks, looks good! Since we are introducing a breaking change in Helm charts between 1.9 and this PR (1.10?), I would recommend documenting it directly in the commit. Could you add a mention for that in commit description? Something along the lines of:

Breaking change: Helm releases previously deployed with additional arguments in object-based values.extraArgs will need to update these arguments to the new array-based format.

I suppose we may also need to add the breaking change to the release notes and/or upgrade guide (Documentation/operations/upgrade.rst), though I'm not sure about the process for either. @joestringer any input?

[joestringer]

The "BPF checks" failure is really checking the commit description and is complaining about the format of the message. Typically in Cilium we will use a format like this for commit messages:

helm: Replace object-based extraArgs with array-based

Breaking change: Helm releases previously deployed with additional arguments in object-based "values.extraArgs", but this prevented providing the same parameter multiple times. Replace these arguments to the new array-based format.

Fixes: #15150
Signed-off-by: ...

You can also squash the commits together, I don't think they need to be separate commits. This StackOverflow post goes into more detail.

I suppose we may also need to add the breaking change to the release notes and/or upgrade guide (Documentation/operations/upgrade.rst), though I'm not sure about the process for either. @joestringer any input?

It'd be nice to do this, but the structure is not yet in place since we haven't prepared v1.10 upgrade docs yet. Maybe don't worry about it for now since this PR has release-note/minor which will show up in the release for folks to see. I'll take down a note to set up the v1.10 upgrade notes section and put a note in there for these changes.

[nbusseneau]

Thanks for the changes :) LGTM.

[nbusseneau]

@joestringer Not sure if we should mark this with upgrade-impact or not? On one hand, these options are really meant for development, on the other hand if anybody uses it and upgrades without looking at the release notes his Helm deployment will fail. But even with that said, it will fail at the chart validation stage, so it won't have an impact on the existing Helm deployment...

[joestringer]

@Skymirrh yeah I wouldn't worry too much in this case because the validation should pick it up super early. In general upgrade-impact label is useful to (1) communicate amongst developers that we should consider upgrade implications and figure out how to either address them or document them, or (2) (maybe) later on if there's an upgrade issue, search for all the issues with upgrade-impact and see if any of them might be relevant.

[nbusseneau]

test-me-please

Totally useless for this PR but let's get them green because why not.

[ciliumbot]

Build finished.

[joestringer]

Totally useless for this PR but let's get them green because why not.

Somehow I thought we were using extraArgs in CI, but apparently not.

For what it's worth, if we know that a PR won't be validated by CI because it's too simple or we don't use the functionality (like here), we don't need to run the CI just to try to get some green ticks. It's good enough to post "No need to run CI as no tests use this functionality" (assuming you've validated this is true! 😅 ) . Then janitors/maintainers can recognize why the CI was not run and consider that before merging.

We did have a few of the jobs pass, gke-stable failed to provision and the test-1.2.0-4.9 run appears to have flaked out on one Istio test run (I filed #15337 to follow up). Just waiting on kubernetes codeowners to review /cc @nathanjsweet

[joestringer]

@D1abloRUS did you have some example YAML snippets of how you configured this option? Or did we end up documenting this somewhere? It's a little hard to tell exactly what the correct ConfigMap syntax looks like just from reading the PR.

[nbusseneau]

@D1abloRUS did you have some example YAML snippets of how you configured this option? Or did we end up documenting this somewhere? It's a little hard to tell exactly what the correct ConfigMap syntax looks like just from reading the PR.

Correct syntax is:

extraArgs:
  - --foo=bar
  - --baz=qux

It's not documented anywhere AFAIK, but I personally felt it was OK since it's a developer-centric flag and the {{- toYaml . | trim | nindent 8 }} approach is kinda standard (we have it for a lot of list objects in the charts).

Do you think we should document it?

[michi-covalent]

for some settings we just have a sample configuration values commented out:

cilium/install/kubernetes/cilium/values.yaml

Line 177 in c95f6fd

resources: {}

this way at least you can kind of see how the helm value is supposed to look like.