zendesk / samson

Web interface for deployments, with plugin architecture and kubernetes support
Other
1.45k stars 234 forks source link

feat: allow setting kubernetes well-known labels #4027

Closed a7i closed 2 years ago

a7i commented 2 years ago

Signed-off-by: Amir Alavi amir.alavi@zendesk.com

For PR description, please view README addition. For comparison, other deployments tools such as Spinnaker add these two labels as well. See Doc here

References

N/A

Risks

a7i commented 2 years ago

won't this give us lots of broken names ? (since we just set whatever the name is to the name field and that's not consistent)

I'm not too familiar with the code base, would you elaborate? What should it be set to?

grosser commented 2 years ago

I don't think what we want is to put every metadata.name into the canonical name field. (If it were that easy k8s would already do it) We want the apps name, which afaik is the project label. ... and for those where it's not the project label the naming would be random if we just guessed it. So for example a project might have a name, but the deployment is named "foo-worker", but that should not go into the name field.

So: if we want to set it for consistency: use the project label. if we want to set it because the project label is not good enough: better just not set it automatically.

a7i commented 2 years ago

I don't think what we want is to put every metadata.name into the canonical name field. (If it were that easy k8s would already do it) We want the apps name, which afaik is the project label. ... and for those where it's not the project label the naming would be random if we just guessed it. So for example a project might have a name, but the deployment is named "foo-worker", but that should not go into the name field.

So: if we want to set it for consistency: use the project label. if we want to set it because the project label is not good enough: better just not set it automatically.

project label makes sense but in that case, we could also set app.kubernetes.io/instance (to role). project and role are "proprietary" labels only understood by Samson or in-house custom tooling. app.kubernetes.io/name is a canonical label well understood by other software in the k8s space (e.g. Istio). This is why Helm, Spinnaker, Tekton (and others) automatically add this label so then it raises the question as to why shouldn't Samson?

grosser commented 2 years ago

Does helm set different labels when a project has multiple components ? I just don't want us to set random values in bless there is a clear benefit for setting something.

On Sun, Aug 28, 2022, 1:29 PM Amir Alavi @.***> wrote:

I don't think what we want is to put every metadata.name into the canonical name field. (If it were that easy k8s would already do it) We want the apps name, which afaik is the project label. ... and for those where it's not the project label the naming would be random if we just guessed it. So for example a project might have a name, but the deployment is named "foo-worker", but that should not go into the name field.

So: if we want to set it for consistency: use the project label. if we want to set it because the project label is not good enough: better just not set it automatically.

project label makes sense but in that case, we could also set app.kubernetes.io/instance (to role). project and label are "proprietary" labels only understood by Samson or in-house custom tooling. app.kubernetes.io/name is a canonical label well understood by other software in the k8s space (e.g. Istio https://github.com/istio/istio/blob/4cb2fcf9195138ce331d0120b7e37b8fa5add1b2/pkg/kube/labels/labels.go#L26). This is why Helm, Spinnaker, Tekton (and others) automatically add this label so then it raises the question as to why shouldn't Samson?

— Reply to this email directly, view it on GitHub https://github.com/zendesk/samson/pull/4027#issuecomment-1229547609, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAACYZYXBTMHKRB6YGBDXATV3PDZTANCNFSM57ZSVN3A . You are receiving this because your review was requested.Message ID: @.***>