Annotations and Labels #4
Conversation
Signed-off-by: Mathieu Benoit <mathieu-benoit@hotmail.fr>
Signed-off-by: Mathieu Benoit <mathieu-benoit@hotmail.fr>
mathieu-benoit
force-pushed
the
deployment-annotations
branch
from
b879928
to
80bdf5b
Compare
Signed-off-by: Mathieu Benoit <mathieu-benoit@hotmail.fr>
sujaya-sys
requested review from
chrishumanitec,
johanneswuerbach and
astromechza
| @@ -2,16 +2,22 @@ apiVersion: apps/v1 | |||
| kind: Deployment | |||
| metadata: | |||
| name: {{ include "workload.name" . }} | |||
| annotations: | |||
| {{- toYaml .Values.workload.annotations | nindent 4 }} | |||
There was a problem hiding this comment.
We really should add these to the values.yaml file as sample defaults.
workload:
annotations: {}
labels: {}
containers:
annotations: {}
labels: {}
service:
labels: {}
Why no annotations on the service?
There was a problem hiding this comment.
RE: values.yaml file as sample defaults - love the idea, but it's not the case today: https://github.com/score-spec/score-helm-charts/blob/main/charts/workload/values.yaml - useless for now, but let's add in another/dedicated PR?
There was a problem hiding this comment.
Why no annotations on the service?
No reasons, I missed it, just added, thanks for catching this.
There was a problem hiding this comment.
Can you provide more context on this change? Why is it needed? what does it benefit?
Are you going to add to the conversion function in https://github.com/score-spec/score-helm/blob/main/internal/helm/convert.go#L42C8-L42C8 to enable this?
There was a problem hiding this comment.
Can you provide more context on this change? Why is it needed? what does it benefit?
I just updated the main description of this PR for the use cases. Let me know if you need more information.
There was a problem hiding this comment.
Are you going to add to the conversion function in https://github.com/score-spec/score-helm/blob/main/internal/helm/convert.go#L42C8-L42C8 to enable this?
Nop, it's not related to that. These labels and annotations are not part of the Score spec anyway.
This PR is about this workflow:
score-helm run \
-f score.yaml \
-o score-helm-values.yaml
helm install \
test \
--repo https://score-spec.github.io/score-helm-charts \
workload \
--values score-helm-values.yaml \
--set "workload.containers.annotations.dapr\.io/enabled=true"
So just about the second command (helm install), not the first one (score-helm run).
Let me know if you need more information around this.
Signed-off-by: Mathieu Benoit <mathieu-benoit@hotmail.fr>
|
Thanks for the review @astromechza! @chrishumanitec does this look ok to you? If so we could go ahead merge & issue a new release. Thanks for checking 🙂 |
Ability to add
labelsandannotationsto bothDeploymentandService.What does this change do?
This PR add a way to add
labelsandannotationson both theDeploymentand theService.Here are some use cases:
--set "workload.annotations.radapp\.io/enabled=true"- example here: https://github.com/mathieu-benoit/score-radius--set "workload.containers.annotations.dapr\.io/enabled=true"In order to accomplish this as an example:
Have you read the Contributing Guidelines?