Custom labels added for istio-cni deployment and pods in helm chart #50694
Conversation
|
😊 Welcome @nischay30! This is either your first contribution to the Istio istio repo, or it's been You can learn more about the Istio working groups, Code of Conduct, and contribution guidelines Thanks for contributing! Courtesy of your friendly welcome wagon. |
istio-testing
added
the
size/S
|
Hi @nischay30. Thanks for your PR. I'm waiting for a istio member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the 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/test-infra repository. |
| @@ -32,6 +32,12 @@ defaults: | |||
| # Allows user to set custom affinity for the DaemonSet | |||
| affinity: {} | |||
|
|
|||
| # Custom labels on Deployment level, if you need them | |||
There was a problem hiding this comment.
This isn't a Deployment so the naming is kind of weird?
There was a problem hiding this comment.
I was about to open a PR - can you add the "app: istio-cni" label as well ?
It is very annoying that all other charts have this label - but this one uses k8s-app instead ( and I don't know what "-node" suffix means - all other charts use the "app: chart_name")
There was a problem hiding this comment.
And it may be useful to provide an example of 'when you need them' - by the criteria that you may need something we would replicate the entire Pod spec in values.yaml...
I know there is a pattern of redundant docs in most of the helm charts, but at least for new stuff it may be nice to have at least an example use case.
The |
Do you have any example? any format? where we can add? because custom labels isn't a big thing |
|
Not change - add. There is no harm in adding an extra label.
…On Mon, Apr 29, 2024, 03:45 Nischay Goyal ***@***.***> wrote:
I was about to open a PR - can you add the "app: istio-cni" label as well ?
It is very annoying that all other charts have this label - but this one
uses k8s-app instead ( and I don't know what "-node" suffix means - all
other charts use the "app: chart_name")
The k8s-app label is present in spec.selector.matchLabels, which is
immutable field. By replacing, it will become a breaking change, where
first it needs to be uninstalled.
—
Reply to this email directly, view it on GitHub
<#50694 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAUR2XRRTNNQ7TX6YIOYJLY7YQDDAVCNFSM6AAAAABG2QGFCGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAOBSGQYDCNBZGY>
.
You are receiving this because your review was requested.Message ID:
***@***.***>
|
|
Don't worry about 'app', I can open a separate PR.
For comments - just some example like what label you are planning to use, I
assume this PR is based on some use case you have.
…On Mon, Apr 29, 2024, 15:50 Costin Manolache ***@***.***> wrote:
Not change - add. There is no harm in adding an extra label.
On Mon, Apr 29, 2024, 03:45 Nischay Goyal ***@***.***>
wrote:
> I was about to open a PR - can you add the "app: istio-cni" label as well
> ?
>
> It is very annoying that all other charts have this label - but this one
> uses k8s-app instead ( and I don't know what "-node" suffix means - all
> other charts use the "app: chart_name")
>
> The k8s-app label is present in spec.selector.matchLabels, which is
> immutable field. By replacing, it will become a breaking change, where
> first it needs to be uninstalled.
>
> —
> Reply to this email directly, view it on GitHub
> <#50694 (comment)>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAAUR2XRRTNNQ7TX6YIOYJLY7YQDDAVCNFSM6AAAAABG2QGFCGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAOBSGQYDCNBZGY>
> .
> You are receiving this because your review was requested.Message ID:
> ***@***.***>
>
|
It's good practice to have both, like we already do in the ztunnel daemonset chart. My biggest beef with the |
|
On Thu, May 2, 2024 at 9:02 AM Ben Leggett ***@***.***> wrote:
Can you add a release note file?
Also explain why use daemonset vs pod level label for istio-cni?
cc @bleggett <https://github.com/bleggett>
It's good practice to have both, like we already do in the ztunnel
daemonset chart.
My biggest beef with the istio-cni chart is the use of the cni prefix for
all the values, which is an artifact of istioctl and very non-helm
+1
There is an opportunity to cleanup a bit - add new things at top level, add
"public / stable" things to top level, etc.
|
istio-testing
added
the
needs-rebase
istio-testing
removed
the
needs-rebase
I have changed to labels as discussed above. Also, how to add release-notes file? |
Check |
This PR adds the capability in helm chart for adding custom labels to
istio-cnideployments ands pods and aims at this Issue - #50372