Alow fixed custom labels in any pod #2646
Conversation
claudio4j
force-pushed
the
feat_additional_labels
branch
from
97aa688
to
0699ecb
Compare
claudio4j
force-pushed
the
feat_additional_labels
branch
from
0699ecb
to
8ca0ad7
Compare
pkg/util/label/label.go
Outdated
| func AddLabels(integration string) map[string]string { | ||
| AdditionalLabels = strings.ReplaceAll(AdditionalLabels, "@integration@", integration) | ||
| AdditionalLabels = fmt.Sprintf("%s=%s,%s", v1.IntegrationLabel, integration, AdditionalLabels) | ||
| lbls, _ := labels.ConvertSelectorToLabelsMap(AdditionalLabels) |
There was a problem hiding this comment.
Instead of swallowing the error, the parsing could be call in an init method, that would panic in case the labels are ill -defined.
There was a problem hiding this comment.
I choose not to panic or throw the error up, as these additional labels are not critical for the integrations to run, but would be interesting to at least print a warning in the log.
There was a problem hiding this comment.
I think I've just experienced why fail-fast error handing is generally preferred. I tried to understand why the tests were failing, and it took me half an hour, scratching my head, to realise that there was a trailing comma in the string, among the zillion log statements printed in the e2e test suite execution, like the needle in a haystack. We are lucky Kubernetes caught the error, but that's the kind of things that could be hard to detect, and panicking would make it immediate.
There was a problem hiding this comment.
Let me precise my though, this is a variable set at build time, so the closer we fail to the build the better. Panicking at the operator start time is the closest place I can think of. It's correct it's not critical for the integrations, but it's rather critical for the build. As with any feedback loop, the closer the the consequence from the cause the better.
There was a problem hiding this comment.
ok, I will panic then. Can you share which e2e failed, so I can reproduce ?
There was a problem hiding this comment.
One occurrence is available at: https://github.com/apache/camel-k/runs/3661980872?check_suite_focus=true#step:10:9045.
There was a problem hiding this comment.
I hadn't run the e2e tests before, good you had catch this one. just pushed the changes and hope it may be fixed now. Thanks.
claudio4j
force-pushed
the
feat_additional_labels
branch
from
8ca0ad7
to
4122921
Compare
claudio4j
force-pushed
the
feat_additional_labels
branch
from
4122921
to
07c2f40
Compare
pkg/util/label/label.go
Outdated
| lbls, err := labels.ConvertSelectorToLabelsMap(strLabels) | ||
| if err != nil { | ||
| // as this should be used only in build time, it's ok to fail fast | ||
| panic(fmt.Sprintf("Error parsing AdditionalLabels %s, Error: %s\n", strLabels, err)) |
There was a problem hiding this comment.
Sorry I may not have been clear, but I wasn't suggesting to panic here. The idea I suggested is to do the parsing in an init function to set a labels map variable, and panic if the parsing fails there. The parsing only needs to happen once, at start time, and it fails fast if the additional labels string isn't valid.
|
There are two failing tests of |
Yes, the |
claudio4j
force-pushed
the
feat_additional_labels
branch
from
07c2f40
to
f778116
Compare
|
Let's merge it as the OpenShift workflow test error is unrelated. |
Fix apache#2646 In relation to ENTESB-14406 Provide metering labels for Camel K Operator and deployed pods https://issues.redhat.com/browse/ENTESB-14406
Fix for #2642
This change enables to add custom labels to camel-k-operator at build time, allowing to set custom fixed labels which will be set on any pod.
The developer should append this line to the
Makefileto set theAdditionalLabelsvariable with the desired values, they are a map of key=values, separated by semi-colon (;) allowed characters are defined in kubernetes doc.There is a special case, if the user wants to use the integration name, just put the value as
token_integration_name, see example below. In this case, the pod will contains a label whose value is the integration name defined in runtime.Release Note