Add extra containers to helm chart

[sagikazarmark]

Proposed changes

Add extra containers to helm chart to allow injecting sidecar containers

Checklist

Before creating a PR, run through this checklist and mark each as complete.

• I have read the CONTRIBUTING doc
• I have added tests that prove my fix is effective or that my feature works
• I have checked that all unit tests pass after adding my changes
• I have updated necessary documentation
• I have rebased my branch onto master
• I will ensure my PR is targeting the master branch and pulling from my branch from my own fork

[codecov-commenter]

Codecov Report

Merging #2294 (6a2ad4f) into master (3178c54) will decrease coverage by 0.03%.
The diff coverage is n/a.

❗ Current head 6a2ad4f differs from pull request most recent head 20239c4. Consider uploading reports for the commit 20239c4 to get more accurate results

@@            Coverage Diff             @@
##           master    #2294      +/-   ##
==========================================
- Coverage   53.69%   53.66%   -0.04%     
==========================================
  Files          48       48              
  Lines       14203    14201       -2     
==========================================
- Hits         7627     7621       -6     
- Misses       6338     6340       +2     
- Partials      238      240       +2     

Impacted Files	Coverage Δ
...ternal/k8s/appprotect/app_protect_configuration.go	86.16% <0.00%> (-0.58%)	⬇️
internal/k8s/configuration.go	95.47% <0.00%> (-0.39%)	⬇️
cmd/nginx-ingress/main.go	6.31% <0.00%> (+0.02%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3178c54...20239c4. Read the comment docs.

[sagikazarmark]

Hey folks!

Happy new year!

It looks like one of the tests is breaking, but I don't really know why (it wasn't breaking before). Can I get some guidance on how to fix it? And maybe a review as well?

Thanks!

[ciarams87]

Hi @sagikazarmark - sorry for the delay in response. Thanks for the PR! This looks good - could you just please also add an entry for the new field to the Configuration table in the Helm README - https://github.com/nginxinc/kubernetes-ingress/blob/master/deployments/helm-chart/README.md#configuration - we should be good to go then. Thanks!

[sagikazarmark]

@ciarams87 Sorry for the delay as well. Your comment got lost in the nginx-bot messages. Updating the readme now!

[sagikazarmark]

@ciarams87 updated the readme. Let me know if I should make any more changes. Thanks!

[sagikazarmark]

Friendly ping :)

[pleshakov]

Hi @sagikazarmark

Looks good!

Could you possibly add a line about the new parameter here as well https://github.com/nginxinc/kubernetes-ingress/blob/master/docs/content/installation/installation-with-helm.md?plain=1#L181 for documentation purposes?

Could you also rebase your branch against the latest master?

After that, we will happily merge!

Thank you

[sagikazarmark]

@pleshakov Thanks! I made the requested changes.

I checked the contributing guide for instructions about documentation changes, but I couldn't find any (I could have missed them). It might make sense to mention adding new values to the helm chart.

Here is an example from Dex's contribution guide (although documentation works a bit differently there).

Thanks again for your review!

[flaper87]

Just wanted to drop a comment and thank @sagikazarmark for working on this! This will make it easier for some architectures to extend the nginx deployment without custom images! 🙏