korroot commented 3 years ago

Add component as a label, not only annotation to Kubernetes objects.

Add componentLabel as CLI parameter for backward compatibility

In order to decide if add component name as label to k8s objects, there is a CLI parameter called --component-label. By default it is set to false, and only after passing --component-label argument to qbec it adds 'qbec.io/component' as label.

This addresses #180

codecov-io commented 3 years ago

Codecov Report

Merging #186 (f8241c9) into master (3ef9f8e) will increase coverage by 0.08%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #186      +/-   ##
==========================================
+ Coverage   73.30%   73.38%   +0.08%     
==========================================
  Files          51       51              
  Lines        4252     4265      +13     
==========================================
+ Hits         3117     3130      +13     
  Misses        961      961              
  Partials      174      174

Impacted Files	Coverage Δ
internal/commands/apply.go	`79.47% <100.00%> (+0.41%)`	:arrow_up:
internal/commands/config.go	`89.30% <100.00%> (+0.11%)`	:arrow_up:
internal/commands/diff.go	`85.51% <100.00%> (+0.20%)`	:arrow_up:
internal/commands/show.go	`92.03% <100.00%> (+0.21%)`	:arrow_up:
internal/eval/eval.go	`94.61% <100.00%> (ø)`
internal/model/k8s.go	`90.00% <100.00%> (+0.41%)`	:arrow_up:
internal/remote/pristine.go	`77.21% <100.00%> (ø)`
internal/types/secrets.go	`87.50% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 3ef9f8e...f8241c9. Read the comment docs.

korroot commented 3 years ago

@gotwarlost @harsimranmaan any thoughts on my PR?

gotwarlost commented 3 years ago

Sorry for the delay in response. I've been pretty slammed with other things.

Some history - The reason component is an annotation and not a label is that we never want to use it as a label selector in the qbec GC flow and my idea was that we should prefer annotations to labels when possible since label values have length and character restrictions. It is ok to say that environment and application name should be valid qbec labels but it seemed unreasonable to me that we should introduce such restrictions on component naming especially since qbec doesn't need it.

That said, I'm open to an PR that also adds the component label as long as it doesn't break the qbec interface and is 100% opt-in. This PR is that so it satisfies these criteria.

However, I think the implementation should not be a command line flag that people need to remember to set for every invocation but a boolean attribute in qbec yaml (say, addComponentLabel defaulting to false). This will ensure that the component label is always even set when different people run the command. It really is a property of the qbec app and not an "option".

gotwarlost commented 3 years ago

Change the swagger schema: https://github.com/splunk/qbec/blob/master/internal/model/swagger.yaml
Regenerate the JSON using gen-qbec-swagger (we should probably add a make target to facilitate this)
Add the corresponding field to the QbecApp object
Add a method to the App object that wraps the QbecApp object
Use it where needed

gotwarlost commented 3 years ago

Oh and add it to the reference doc page: https://github.com/splunk/qbec/blob/master/site/content/reference/qbec-yaml.md

korroot commented 3 years ago

Thanks for review. I like the idea about config file where to set if component should be able added as a label. I will work on the comments and return in some 2 weeks (after New Year).

hudymi commented 3 years ago

@gotwarlost I have contacted @korroot, and I will continue the work on this. Please take a look if the provided changes are something that you expected as a valid solution. Thanks :smile:

gotwarlost commented 3 years ago

LGTM. The only issue I see is that we have made an already ugly signature of model.NewK8sLocalObject even uglier by adding the bool. That said, we can fix that as a refactor in a later PR.

I'm merging this.

gotwarlost commented 3 years ago

Now available in the v0.13.3 release: https://github.com/splunk/qbec/releases/tag/v0.13.3

Thank you @korroot and @hudymi for your contribution and happy holidays!

splunk / qbec

Add qbec.io/component as label #186

Codecov Report