Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update labels list for Cluster API #29294

Closed
wants to merge 1 commit into from

Conversation

nawazkh
Copy link
Contributor

@nawazkh nawazkh commented Apr 12, 2023

This PR updates the list of labels to be added to issues and PRs at https://github.com/kubernetes-sigs/cluster-api.

Part of kubernetes-sigs/cluster-api#8263

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 12, 2023
@k8s-ci-robot k8s-ci-robot added area/label_sync Issues or PRs related to code in /label_sync sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Apr 12, 2023
@@ -1814,6 +1814,144 @@ repos:
target: both
prowPlugin: label
addedBy: anyone
- color: 0052cc
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there's a couple of labels to remove from above:

area/ux
area/artifacts
area/provider/docker
area/operator

and a couple of more. The list here should have the same number of items as the list in the doc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does that mean we drop labels like kind/proposal ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No - This should be scoped to only area labels, let's not touch any of the labels with another prefix in the PR.

@@ -1814,6 +1814,144 @@ repos:
target: both
prowPlugin: label
addedBy: anyone
- color: 0052cc
description: Issues or PRs related to CAPBK
name: area/kubeadm-bootstrap
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be bootstrap-kubeadm and below control-plane-kubeadm

➜ k get provider -A
NAMESPACE                           NAME                    AGE   TYPE                     PROVIDER      VERSION
capd-system                         infrastructure-docker   37m   InfrastructureProvider   docker        v1.4.99
capi-kubeadm-bootstrap-system       bootstrap-kubeadm       43m   BootstrapProvider        kubeadm       v1.4.99
capi-kubeadm-control-plane-system   control-plane-kubeadm   43m   ControlPlaneProvider     kubeadm       v1.4.99
capi-system                         cluster-api             43m   CoreProvider             cluster-api   v1.4.99

I wonder if "provider" would be helpful as well (also for infrastructure-docker)

Maybe

area/provider/infrastructure-docker
area/provider/bootstrap-kubeadm
area/provider/control-plane-kubeadm
area/provider/core

(not sure if we don't like the provider segment)

or:

area/infrastructure-provider-docker
area/bootstrap-provider-kubeadm
area/control-plane-provider-kubeadm
area/core-provider

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@killianmuldoon separate discussion. Maybe we should start a discussion around moving the core provider stuff into a core folder. This would make the structure of our repository a lot easier
(we can keep the core APIs where they are if folks are concerned about moving them, although all the core APIs which are not already in the top-level api folder will have to move sooner or later anyway)

Related, maybe we should re-structure a bunch more stuff in our repo.

Copy link
Contributor

@killianmuldoon killianmuldoon Apr 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On changing the label naming here - to add "provider" either as a path prefix or elsewhere - I don't think it really helps or hinders. Just happy to pick any form and stick to it.

Agree with moving stuff into a core folder, but this probably isn't the place to have a good discussion on it 😄

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer if we have provider in the label name. If we don't want an additional segment we can use the form without, otherwise I would pick the ones in this format: "area/provider/infrastructure-docker"

Definitely just wanted to bring up the topic :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added area/provider/core
updated labels as

area/provider/infrastructure-docker
area/provider/bootstrap-kubeadm
area/provider/control-plane-kubeadm

label_sync/labels.yaml Show resolved Hide resolved
label_sync/labels.yaml Show resolved Hide resolved
target: both
prowPlugin: label
addedBy: anyone
- color: 0052cc
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to the comment in the doc. Seems like now we have clusterclass and topology

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah - I'm happy to pick one or the other. Maybe ClusterClass as it's clearer for release notes?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think ClusterClass is more expressive

topology could be so much more or something very different as ClusterClass

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to using clusterclass. Shall I be updating to topology instead?

Copy link
Member

@sbueringer sbueringer Apr 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No updating to clusterclass is fine. I just meant that topology is too generic.

addedBy: anyone
- color: 0052cc
description: Issues or PRs related to devtools
name: area/devtools
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't find it in the doc. What do we see as devtools, all the tilt stuff?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tilt stuff, any other helper scripts etc. I think.

Copy link
Member

@sbueringer sbueringer Apr 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe would be good to give a hint in the label description. Although I'm a bit nitpicky here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kept it as is for now unless you want me to remove it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's fine let's keep it as is

@sbueringer
Copy link
Member

sbueringer commented Apr 26, 2023

/hold

I want to run the label sync with dry-run locally before we merge this PR.

I don't want to risk unintended side effects

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 26, 2023
Comment on lines 1690 to 1702
- color: c7def8
description: Issues or PRs related to proposals.
name: kind/proposal
target: both
prowPlugin: label
addedBy: anyone
- color: f6c5d8
description: Issues or PRs that need to be closed before the next CAPI release
name: kind/release-blocking
target: both
prowPlugin: label
addedBy: approvers
- color: 0052cc
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we don't want to drop those

@killianmuldoon
Copy link
Contributor

Hey @nawazkh - just did a round of review on this with @sbueringer. We found out there's a couple of gotchas with removing labels as we're not sure of the syncing between this list and the CAPI repo. We also did another round on the label google document and created a new list - https://docs.google.com/document/d/1PWUZsoO-4Un4FHszEeMDCF_007ic6uA0WRj-crMhB3A/edit#

For this PR I think the best route forward is:

  1. don't drop any labels in this PR
  2. Add any labels from the google doc which don't exist in this list in this PR

Then in a follow-up we can remove the labels we don't want. How does that sound to you?

@nawazkh
Copy link
Contributor Author

nawazkh commented Apr 28, 2023

For this PR I think the best route forward is:

  1. don't drop any labels in this PR
  2. Add any labels from the google doc which don't exist in this list in this PR

Sounds good to me!

@nawazkh
Copy link
Contributor Author

nawazkh commented May 2, 2023

/test pull-test-infra-verify-lint

@nawazkh nawazkh force-pushed the add_area_labels_capi branch 2 times, most recently from b414217 to 078a9f1 Compare May 2, 2023 06:52
@nawazkh
Copy link
Contributor Author

nawazkh commented May 2, 2023

  • Removed area/dependency as pull-test-infra-verify-lint was failing with the error duplicate label area/dependency at kubernetes-sigs/cluster-api.area/dependency and default.area/dependency.
  • area/dependency is already part of default section.

Copy link
Contributor

@killianmuldoon killianmuldoon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me pending running the make target.

@CecileRobertMichon in case you want to take another look as there's been a couple of changes since you initially reviewed the doc.

@sbueringer also ready for your review for the added labels.

- remove area/dependency since it is present is default section
@sbueringer
Copy link
Member

Thx!

looks good

/lgtm
/approve
/hold cancel

Please feel free to continue the reviews and we can do a follow-up PR if necessary

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 3, 2023
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 3, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: nawazkh, sbueringer
Once this PR has been reviewed and has the lgtm label, please assign spiffxp for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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

@sbueringer
Copy link
Member

/assign @cblecker

@killianmuldoon
Copy link
Contributor

/close

In favor of #29421

@k8s-ci-robot
Copy link
Contributor

@killianmuldoon: Closed this PR.

In response to this:

/close

In favor of #29421

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/label_sync Issues or PRs related to code in /label_sync cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants