kubebuilder icon indicating copy to clipboard operation
kubebuilder copied to clipboard

:sparkles: (kustomize/v1 and kustomize/v2-alpha): add labels on the manifests following the k8s recommendations

Open everettraven opened this issue 3 years ago • 13 comments
trafficstars

Description of Change

update or add labels in the kustomize plugin(s) scaffolded files to match the k8s recommended labels

Motivation for Change

resolves #2773

everettraven avatar Jul 05 '22 20:07 everettraven

Skipping CI for Draft Pull Request. If you want CI signal for your change, please convert it to an actual PR. You can still manually trigger a test run with /test all

k8s-ci-robot avatar Jul 05 '22 20:07 k8s-ci-robot

/hold

everettraven avatar Jul 05 '22 20:07 everettraven

/cc @camilamacedo86

everettraven avatar Jul 05 '22 20:07 everettraven

@camilamacedo86 I will make sure to squash the commits before merge, but wanted to keep as individual commits until all the labels we want are in place and there are only minor adjustments to be made.

everettraven avatar Jul 08 '22 16:07 everettraven

/retest

everettraven avatar Jul 08 '22 17:07 everettraven

@everettraven for this PR because it is so large, it would be helpful to give a synopsis TL;DR of what the change is doing.

jmrodri avatar Jul 13 '22 04:07 jmrodri

Hi @jmrodri,

@everettraven for this PR because it is so large, it would be helpful to give a synopsis TL;DR of what the change is doing.

It is big because we are adding the labels to follow the k8s recommendations in all templates and it will update ALL samples. Following is an example with comments from the definition in the k8s docs:

metadata:
  labels:
    app.kuberentes.io/name: issuer // The name of the application ( in this case the Kind )
    app.kubernetes.io/instance: selfsigned-issuer // A unique name identifying the instance of an application (CR)
    app.kubernetes.io/component: certificate // The component within the architecture
    app.kubernetes.io/created-by: {{ .ProjectName }} // The controller/user who created this resource (in our case it is not created by a controller and it is manage by the kind )
    app.kubernetes.io/part-of: {{ .ProjectName }} // The name of a higher level application this one is part of
    app.kubernetes.io/managed-by: cert-manager.io // The tool being used to manage the operation of an application - @everettraven this one is wrong. The tool in the example is helm. Here we manage the manifest with kustomize. 

But, note that GitHub allow you to check the directories which have manifest changed and you can ignore all under the testdata in this case ( because these files are the samples changed ):

Screenshot 2022-07-13 at 08 39 28

How to review? a) Check the doc: https://kubernetes.io/docs/concepts/overview/working-with-objects/common-labels/ b) Check the manifests changed and the labels ( ignoring the testdata ) and see if you agree with the usage/values adopted by the labels.

c/c @everettraven

camilamacedo86 avatar Jul 13 '22 07:07 camilamacedo86

Hi @everettraven,

The app.kubernetes.io/managed-by: cert-manager.io is wrong, see the docs that is the Tool that manages the manifest in our case the manifests can only be managed with kustomize or with the project itself via the controllers. the cert-manager.io is NOT a tool it is a service.

Screenshot 2022-07-13 at 08 48 37 Screenshot 2022-07-13 at 08 48 44

So, we should here add kustomize for all manifest under config/ because that is the tool ( such as helm ) that is used to build the files.

/hold

camilamacedo86 avatar Jul 13 '22 07:07 camilamacedo86

Hi @everettraven

For we get this one merged we need to squash the commits? Could you please squash them?

/hold

camilamacedo86 avatar Jul 19 '22 14:07 camilamacedo86

@camilamacedo86 commits squashed!

everettraven avatar Jul 19 '22 14:07 everettraven

@camilamacedo86 the code coverage failure seems to be unrelated to this PR and may just need a re-run. Would you be able to re-run it? I don't seem to have permissions to do so.

everettraven avatar Jul 19 '22 15:07 everettraven

@camilamacedo86 I rebased this PR and I think it should be good to go now. Would you mind giving it another look so we can get it merged? Thanks!

everettraven avatar Aug 19 '22 13:08 everettraven

/unhold

everettraven avatar Aug 24 '22 20:08 everettraven

/cc @varshaprasad96 @jmrodri

everettraven avatar Aug 26 '22 13:08 everettraven

@camilamacedo86 I rebased this PR and I think it should be good to go now. Would you mind giving it another look so we can get it merged? Thanks!

@camilamacedo86 - I just wanted to follow up on this PR. Thanks!

everettraven avatar Aug 26 '22 13:08 everettraven

The one thing that would have made this easier to review is if all of the testdata files were a single commit and the main changes were another. Then I could take a quick look at testdata but focus on the core changes. Just an FYI for next time.

jmrodri avatar Aug 30 '22 13:08 jmrodri

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: AlmogBaku, camilamacedo86, everettraven

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot avatar Aug 30 '22 14:08 k8s-ci-robot