Use annotations instead of labels on created k8s resources #4466
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4466 +/- ##
==========================================
- Coverage 72.26% 72.25% -0.02%
==========================================
Files 331 333 +2
Lines 12863 12988 +125
==========================================
+ Hits 9296 9384 +88
- Misses 2975 3012 +37
Partials 592 592
Continue to review full report at Codecov.
|
| @@ -68,7 +68,7 @@ spec: | |||
| expectedOut: `apiVersion: v1 | |||
| kind: Pod | |||
| metadata: | |||
| labels: | |||
| annotations: | |||
There was a problem hiding this comment.
shd we still have these annotations? We are not using any of the annotations for driving any metrics.
With the new MetadataEvent we are capturing all these metrics via IDEs
There was a problem hiding this comment.
I remember these were initially added to get some metrics on how folks are using skaffold. I might be wrong.
I don't think any of our users are using these labels to query objects. Removing these won't be a backward compatible change.
We also have --label field for users to optionally add user labels.
WDYT?
There was a problem hiding this comment.
I think you meant removing them won't be a backwards incompatible change? we're already removing them as labels which would theoretically break people who are using those labels to filter resources, but I think we're fine to do that.
curious point though as to whether we should even have them. putting them as annotations makes them silent metadata, so I'm inclined to just leave them even though they may be useless, but I also don't care much and can remove them if you think we should
There was a problem hiding this comment.
I think you meant removing them won't be a backwards incompatible change?
Correct.
curious point though as to whether we should even have them. putting them as annotations makes them silent metadata, so I'm inclined to just leave them even though they may be useless, but I also don't care much and can remove them if you think we should
Removing them could simplify the code a lot. We don't need the Annotations method in the interface. Merging annotations etc is not needed.
|
thinking about this a little more i think we could remove a lot of code if we get rid of the unnecessary annotations. going to close this PR and reopen another shortly |
fixes #3133
this will still leave two labels on created resources -
skaffold.dev/run-id, andapp.kubernetes.io/managed-by=skaffold. this does remove the version from themanaged-bylabel though, so changing skaffold version will not cause issues.