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

Clean up the existing area labels and adding new useful ones #8263

Closed
joekr opened this issue Mar 10, 2023 · 20 comments
Closed

Clean up the existing area labels and adding new useful ones #8263

joekr opened this issue Mar 10, 2023 · 20 comments
Labels
triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@joekr
Copy link
Member

joekr commented Mar 10, 2023

As a part of the PR template change (#8260) we are asking folks to add /area label(s). This is an iterative approach to improve change tracking. When PRs are labeled by area the release note generation can be more automated (issue where area labels were first discussed #8051). In order to achieve this we should clean up our existing labels https://github.com/kubernetes-sigs/cluster-api/labels?q=area and add new labels. These labels should better align with the current prefixes add to the release notes.

For example:
https://github.com/kubernetes-sigs/cluster-api/releases/tag/v1.3.0 has clusterctl: Add move --to-directory and --from-directory flags. The user who created that PR would have used /area clusterctl which currently does exist.

Some new addition suggestions:

  • documentation (or docs)
  • ClusterClass
  • KCP
  • we have testing but what about e2e-testing or test/e2e
  • CI
  • Server-Side-Apply (or SSA)
@k8s-ci-robot k8s-ci-robot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Mar 10, 2023
@killianmuldoon
Copy link
Contributor

killianmuldoon commented Mar 10, 2023

/triage accepted

Thanks for opening this @joekr! To add context we currently have the following labels. I've grouped them based on my own categorization. The existing list is at https://github.com/kubernetes-sigs/cluster-api/labels?q=area

Code area

area/clusterctl: Issues or PRs related to clusterctl
area/bootstrap Issues or PRs related to bootstrap providers
area/control-plane: Issues or PRs related to control-plane lifecycle management
area/machine: Issues or PRs related to machine lifecycle management
area/operator: Issues or PRs related to operator

Features

area/runtime-sdk: Issues or PRs related to Runtime SDK
area/topology: Issues or PRs related to cluster topology

Cross-cutting development areas

area/code-organization: Issues or PRs related to Cluster API code organization
area/api: Issues or PRs related to the APIs
area/health: Issues or PRs related to health
area/ux: Issues or PRs related to UX
area/testing: Issues or PRs related to testing
area/networking: Issues or PRs related to networking
area/security: Issues or PRs related to security
area/upgrades: Issues or PRs related to upgrades
area/dependency: Issues or PRs related to dependency changes
area/release: Issues or PRs related to releasing
area/conformance: Issues or PRs related to Cluster API and Kubernetes conformance tests
area/community-meeting: Issues or PRs that should potentially be discussed in a Kubernetes community meeting.
area/artifacts: Issues or PRs related to the hosting of release artifacts

Providers

area/provider/ibmcloud: Issues or PRs related to ibmcloud provider
area/provider/openstack: Issues or PRs related to openstack provider
area/provider/vmware: Issues or PRs related to vmware provider
area/provider/aws: Issues or PRs related to aws provider
area/provider/azure: Issues or PRs related to azure provider
area/provider/gcp: Issues or PRs related to gcp provider
area/provider/digitalocean: Issues or PRs related to digitalocean provider
area/provider/docker: Issues or PRs related to docker provider

My opinion:

  1. Add more CAPI code area labels
    area/e2e
    area/machinehealthcheck
    area/tilt
    area/utils
    area/machinedeployment
    area/Machinepools
    area/clustercachetracker

  2. Add more cross-cutting development areas
    area/docs
    area/logging
    area/metrics
    area/CI

  3. Consider adding CAPI-repo provider labels
    area/CAPBK
    area/KCP
    area/CAPD

  4. Consider all remove existing /area/provider labels
    area/provider/ibmcloud
    area/provider/openstack
    area/provider/vmware
    area/provider/aws
    area/provider/azure
    area/provider/gcp
    area/provider/digitalocean
    area/provider/docker

  5. Consider removing existing unused / barely used labels
    area/operator
    area/artifacts
    area/community-meeting
    area/conformance
    area/networking

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Mar 10, 2023
@killianmuldoon
Copy link
Contributor

For context, the labels on this repo are set from the list at: https://github.com/kubernetes/test-infra/blob/7471678e74188b7bdd29ba378a13df5a4a47dbb8/label_sync/labels.yaml#L1676

So we can add and remove labels by making the changes at that repo.

@joekr
Copy link
Member Author

joekr commented Mar 10, 2023

Awesome! I can make those changes based on your opinion above and as others provide feedback we can update that repo.

@killianmuldoon
Copy link
Contributor

@joekr Let's leave it a few days at least for other people to have their say 🙂 - I think one thing we don't want is to be making big changes to often, so it would be good to get something like consensus.

@neolit123
Copy link
Member

neolit123 commented Mar 10, 2023

So we can add and remove labels by making the changes at that repo.

i don't think this is needed. iirc an admin can just add the labels to the capi repo and the '/area foo' command can trigger for any existing 'area/foo'

@sbueringer
Copy link
Member

So we can add and remove labels by making the changes at that repo.

i don't think this is needed. iirc an admin can just add the labels to the capi repo and the '/area foo' command can trigger for any existing 'area/foo'

Yup, this should work. While the label sync tool in test-infra is syncing labels it doesn't remove all additional labels that exist in the repo. (had a case recently where I had to delete a label and it wasn't defined in test-infra).

But I personally would prefer if our labels are defined via test-infra vs. an admin configuring them manually.

@sbueringer
Copy link
Member

sbueringer commented Mar 15, 2023

  1. Add more CAPI code area labels

Generally agree, does this already cover ~ everything? (IPAM?)

  • If we have MachineDeployment, do we also need MachineSet? Is that to fine-granular?
  • area/tilt should we make this more generic, something like area/dev-environment?

2.Add more cross-cutting development areas

+1

  1. Consider adding CAPI-repo provider labels

Does core also make sense for the core provider?

Might make sense to use area/provider/ to make clear that thoes are provider labels.

  1. Consider all remove existing /area/provider labels

Agree, let's remove them

  1. Consider removing existing unused / barely used labels

+1 in general. I think barely used might be fine if the label seems to make sense, but I think there are some that we never use and would never use

How do we feel about creating a Google doc with the labels we want with Killians categorization? This way folks can chime in and we can converge towards a "final" (for now) list. Also seems easier to discuss something like this in a Google doc (more interactive).

@fabriziopandini
Copy link
Member

fabriziopandini commented Mar 20, 2023

if the goal is to use this for generating release notes, my main concern is that as of today we are not really applying labels to PRs, and we have to strike a healthy balance between asking for info from all the contributors and keeping the barrier for contributing to CAPI as low as possible.
So I will suggest investigating if there is a way to apply those labels automatically, like e.g what happens in Kubernetes for SIG assignement, and consider if/how to handle PRs impacting more than one area.

WRT to the list of areas, I have only two asks:

  • maintain a clear semantic around what area means: e.g. If area is a vehicle to identify a part of the codebase, let's make sure to be consistent with this and do not use this for different concerns (unfortunately Killian analysis already shows that area is a sort of catch all for many concepts 😞).
  • let's also consider the interlock between area and kind e.g we are currently using kind/documentation for all the doc issues, how this is going to work if we define area/documentation?

@oscr
Copy link
Contributor

oscr commented Mar 20, 2023

if the goal is to use this for generating release notes,

Yes, this is the motivation. Once available we will add it to the release notes tool asap.

my main concern is that as of today we are not really applying labels to PRs, and we have to strike a healthy balance between asking for info from all the contributors and keeping the barrier for contributing to CAPI as low as possible.
So I will suggest investigating if there is a way to apply those labels automatically, like e.g what happens in Kubernetes for SIG assignement, and consider if/how to handle PRs impacting more than one area.

Today when generating the release notes there are sometimes prefixes and for the rest Joe and I use previous release notes / educated guesses what the prefix should be. So not always accurate and somewhat time consuming work that has to be done at the last moment before a release.

I agree both with not pushing this on new contributors and that automation is the best end solution. However as an interim solution could we have it as not required and ask the pr approvers to ensure it's set? They have the domain knowledge required to accurately set it and there is more time before the actual release. Then moving on work towards automating this away more and more. I suggest starting with a gdoc as @sbueringer suggested above and once we have the key areas agreed upon, start labeling.

@oscr
Copy link
Contributor

oscr commented Mar 22, 2023

I've created a PR to add the label area/docs to changes within docs/book: #8341. Let's give it a try and if it works we can expand the discussion on what labels to have and the coverage.

@oscr
Copy link
Contributor

oscr commented Mar 23, 2023

Automatic area assignment worked perfectly #8344.

@joekr
Copy link
Member Author

joekr commented Mar 23, 2023

Sorry it has taking me a bit. Here is a link to a google doc with the all the labels from Killian's comment https://docs.google.com/document/d/1PWUZsoO-4Un4FHszEeMDCF_007ic6uA0WRj-crMhB3A/edit?usp=sharing

@oscr
Copy link
Contributor

oscr commented Mar 27, 2023

There is now also a pr for the release notes tool which uses the area label: #8343. This modified version was used with the upcoming releases this week (1.2, 1.3, 1.4) and it "seems to work:copyright:" when the area label is present.

I think if we can get the most important / easiest areas agreed upon early then it will help a lot for future release notes.

@killianmuldoon
Copy link
Contributor

I've added a few suggestions to the doc. IMO we should define these labels in terms of their intended usage instead of linking it to codebase etc.

I think the area labels should be used (in order of implementation and importance)

  1. Generating release notes
  2. Assigning reviewers
  3. Triage / backward looking analysis

We should automatically label all PRs with area/needed. Right now it's up to reviewers to add labels. For some cases that can be changed - automatic labelling will only work for a subset of the labels IMO , so there's going to be some additional burden on contributors and reviewers.

I think we should merge the label changes soon and make this an incremental process informed by the release team - especially the comms team which will rely on this for generating the notes.

@nawazkh
Copy link
Member

nawazkh commented Apr 12, 2023

Below is a list of areas which do not have a direct correlation to owners file.

- area/machine
- area/machinehealthcheck
- area/util
- area/machinedeployment
- area/machineset
- area/machinepool
- area/clustercachetracker
- area/clusterresourceset
- area/ipam
- area/runtime-sdk
- area/topology
- area/api
- area/networking
- area/security
- area/upgrades
- area/dependency
- area/release
- area/logging
- area/metrics
- area/ci
- area/devtools

As @killianmuldoon mentioned, I agree that it is highly unlikely that the areas will directly correlated with OWNERS file. That got me thinking if we can increase the number of OWNERS file. We can create OWNERS files at a depth of root + 1 level of CAPI code base and define our custom labels logic. This should help comms team, and PRs (prow) to assign appropriate labels as per code changes.

@sbueringer
Copy link
Member

sbueringer commented Apr 14, 2023

I would like to avoid having to manage that many OWNERS files if somehow possible.

I wonder if we should have another round of discussion about further restructuring our repository before we start introducing OWNERS files everywhere.

Some ideas:

  • move core provider stuff in a core folder (we can exclude APIs if folks are concerned about that)
  • get rid of the exp folders for controller code
  • ....

@fabriziopandini
Copy link
Member

I agree with @killianmuldoon and @sbueringer

  • let's get this working incrementally
  • let's focus on the goal of generating release notes
  • let's focus on keeping the resulting release notes as simple as possible to read for the end users
  • let's consider if there are already too many areas in the list being discussed because I'm not sure an excessive level of detail brings much value for the readers of the release notes; it is also most unlikely maintainers and reviewers will do such a granular classification but instead, it creates weird cases where a PR fits in many areas.
  • let's not add owner files. if we are adding owner files for the sake of doing release notes, IMO this is an alarm bell about the overall approach

@killianmuldoon
Copy link
Contributor

This work should be complete now - we can add and remove more labels iteratively as we learn what works and see how the codebase evolves.

@killianmuldoon
Copy link
Contributor

/close

@k8s-ci-robot
Copy link
Contributor

@killianmuldoon: Closing this issue.

In response to this:

/close

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
triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

No branches or pull requests

8 participants