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

Tracking Issue: Normalize Kind Labels #2032

Closed
jberkus opened this issue Apr 11, 2018 · 32 comments
Closed

Tracking Issue: Normalize Kind Labels #2032

jberkus opened this issue Apr 11, 2018 · 32 comments
Labels
sig/contributor-experience Categorizes an issue or PR as relevant to SIG Contributor Experience.

Comments

@jberkus
Copy link
Contributor

jberkus commented Apr 11, 2018

There are several issues open regarding Kind labels. This is a tracking issue to track all of them across the project so that people can visualize what the end result will be.

Issues:

Some general points:

  1. All else being equal, fewer kind labels is considered a good thing.
  2. Kind/ labels which are not explicitly recommended and documented should be considered deprecated.
  3. Old kind labels which have not been used since the 1.9 cycle or earlier should be considered deprecated.
  4. Ideally, issues and PRs should be able to treat Kind labels as exclusive (apply only one per PR/issue), although this should not be enforced due to corner cases.

I am vague on how we should be applying this set of rules to K/K vs. organization-wide. My primary concern is limiting the pool of kind labels in k/k to a relative handful of clearly needed labels.

New list of canonical Kind labels

Deprecated Kind labels:

  • kind/support (to triage/support)
  • kind/enhancement (duplicates kind/feature)
  • kind/technical-debt (duplicates kind/cleanup)

Also officially deprecating these kind labels, which exist but have not been used since 1.9 or earlier:

  • kind/friction (?)
  • kind/mesos-flake (?)
  • kind/old-docs (duplicates kind/documentation)
  • kind/postmortem (?)
  • kind/upgrade-test-failure (duplicates kind/failing-test)

List of Changes To Make

Renames:

  • priority/failing-test to kind/failing-test
  • kind/support to triage/support

New Labels:

  • kind/flake

Existing Labels to be added to labels.md:

  • kind/cleanup
  • kind/design
  • kind/api-change
  • kind/new-api

Finally, announce deprecation of all the deprecated labels above, and mark them as such in the label dictionary.

cc @spiffxp

@k8s-ci-robot k8s-ci-robot added the needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. label Apr 11, 2018
@jberkus
Copy link
Contributor Author

jberkus commented Apr 11, 2018

/sig contributor-experience

@k8s-ci-robot k8s-ci-robot added sig/contributor-experience Categorizes an issue or PR as relevant to SIG Contributor Experience. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Apr 11, 2018
@BenTheElder
Copy link
Member

kind/bug and kind/failing-test seem to commonly overlap since tests fail due to bugs somewhere, why would failing-test not be a priority label still?

@cblecker
Copy link
Member

"bug" tells me that there is a bug in the kubernetes code base somewhere. "failing-test" can be for a bug, but it can also be a test-infra issue, or an external dependency issue unrelated to a bug in the code base.

@BenTheElder
Copy link
Member

"bug" tells me that there is a bug in the kubernetes code base somewhere. "failing-test" can be for a bug, but it can also be a test-infra issue, or an external dependency issue unrelated to a bug in the code base.

This label is also used across the org though? Bugs can also be from code not primarily attributed to a repo (vendor?) or interactions with external tools. failing-test seems like a subset of bug with somewhat murky distinction.

Switching this label away from priority will leave a lot of existing issues / PRs with a now-unused label. I'm not convinced this is helpful FWIW.

@cblecker
Copy link
Member

Switching this label away from priority will leave a lot of existing issues / PRs with a now-unused label.

I don't understand this. Could you explain?

@BenTheElder
Copy link
Member

I don't understand this. Could you explain?

The existing label is pretty heavily used, so switching it will artificially split up issues / PRs into the old label and the new.

@jberkus
Copy link
Contributor Author

jberkus commented Apr 11, 2018

The existing label is pretty heavily used, so switching it will artificially split up issues / PRs into the old label and the new.

So? That's true of any label change. If that's a winning argument, then we can never change any labels ever. The answer to this is to have a bot reassign for currently open issues and PRs.

@spiffxp's issue on this explains the problems with priority/failing-test:

  1. Issues are supposed to have only one priority, but "failing test" always needs to be accompanied by other priorities, and is the only priority where this is the case.
  2. "Failing test" isn't a priority; it doesn't tell us how soon an issue needs to be fixed or a PR needs to be merged. The test could be critical (kubernetes crashes) or it could be trivial (alpha optional component X produces unexpected warnings). "priority/failing-test" doesn't tell us.
  3. At the time the issue is created by the test team, it needs to have a kind/. This pretty much always gets assigned kind/bug, even when that's not appropriate, because how would the test team know? Being able to assign kind/failing-test reflects what we know about the issue at the time, which is that a test is failing.

@jberkus
Copy link
Contributor Author

jberkus commented Apr 11, 2018

Here's a good example of why priority/failing-test is a failure as a label: kubernetes/kubernetes#61645 (comment)

In that issue, it's not even used, and the way we track that it's a failing test is by "[test fail]" in the issue title. When folks start resorting to adding ad-hoc tagging to long text fields, labelling isn't working.

@BenTheElder
Copy link
Member

So? That's true of any label change. If that's a winning argument, then we can never change any labels ever. The answer to this is to have a bot reassign for currently open issues and PRs.

It definitely is true of any label change, and I do think most label changes cause unnecessary churn and confusion FWIW, so I'd prefer strong justification for changing any labels. This is a label we search for and catalog as humans, the bots don't and probably shouldn't care about this label.

The justification from @spiffxp SGTM, but I do want to mention that we still have somewhat awkward overlap with kind/bug and kind/failing-test. What do I label a PR fixing a bug for a failing test? Seems like we will wind up wanting to have two kind label on many PRs/issues instead of two priority labels which doesn't seem like a vast improvement. 🤷‍♂️

Here's a good example of why priority/failing-test is a failure as a label: kubernetes/kubernetes#61645 (comment)
In that issue, it's not even used, and the way we track that it's a failing test is by "[test fail]" in the issue title. When folks start resorting to adding ad-hoc tagging to long text fields, labelling isn't working.

I don't see anything particularly convincing here that renaming this will solve the issue of "people don't use appropriate labels", FWIW. This issue could have just as well been labeled kind/bug [test failure] after this change.

@BenTheElder
Copy link
Member

To clarify I want to see this change overall and love seeing less labels(!) -- I'm not asking to block anything, I'm just raising some concern that those two kind labels will likely continue to be poorly conflated and imho probably raise the likelihood that test failures are only labeled as bugs.

@cblecker
Copy link
Member

@BenTheElder the label sync tool migrates the label. So renaming it doesn't impact current issues/PRs that use the old label.

@jberkus
Copy link
Contributor Author

jberkus commented Apr 12, 2018

@BenTheElder it's not "people don't use appropriate labels", it's "people can't use appropriate labels, because the 'appropriate' labels don't make any sense".

The problem is less that "failing-test ought to be a kind" than "failing-test shouldn't be a priority". That is, having it as a priority causes several different problems, and doesn't make any sense. And if it's not going to be a priority, having it be a kind is what makes the most sense, especially given the workflow considerations mentioned above.

@jberkus
Copy link
Contributor Author

jberkus commented Apr 12, 2018

Also, priority labels are supposed to be exclusive (and this issue is the only reason we don't enforce that), whereas kind labels are "try to use only one". So it is materially better to have two kinds than two priorities, especially when it comes to reporting and triage.

The alternative would be to make failing-test its own, independent, label.

@BenTheElder
Copy link
Member

The alternative would be to make failing-test its own, independent, label.

This might actually make sense. From my point of view a "low priority failing test" doesn't really make sense. What good are tests if we're content to let them fail?
We should fix or eliminate failing tests, otherwise we're just teaching people to ignore them and wasting resources...

@jberkus
Copy link
Contributor Author

jberkus commented Apr 12, 2018

No, but I can see having a failing test be priority/important-soon or priority/important-longterm instead of priority/critical-urgent, depending on the test. For example, new tests for alpha features just aren't critical-urgent when they fail; part of the feature getting to beta is having good tests, but we don't expect the developers to fix the tests on a 48hours-or-less basis.

@spiffxp
Copy link
Member

spiffxp commented Apr 19, 2018

I would rather see N kind/foo labels on an issue than N priority/foo labels.

I hear the kind/bug vs. kind/test-failure point, but I'm thinking of this more from an issue than PR perspective. I would anticipate seeing more kind/bug issues filed by users, vs. kind/test-failure issues being filed by people/bots that watch tests.

I'm explicitly against non-prefixed labels in general, because they break the (IMO intuitive/discoverable) pattern where the /foo bar adds a foo/bar label

It's been a good bit since I've looked at devstats, so I'll survey and see if that helps prove any particular point

@spiffxp
Copy link
Member

spiffxp commented Apr 19, 2018

the label sync tool migrates the label. So renaming it doesn't impact current issues/PRs that use the old label.

Label migration does impact issue / PR update time, but I don't think label renaming does. AFAIK we don't have anything automatically creating issues with priority/failing-test so I wouldn't expect to see the old label reappear if we decided to renaming the existing label.

@cblecker
Copy link
Member

Reading over this discussion, I don't think we have any further objections to moving priority/failing-test to kind/failing-test. I agree that there may be possible confusion with kind/bug, but tbh, that's already the case if we ask reporters to label things. There are many "bugs" that aren't really bugs.

@spiffxp
Copy link
Member

spiffxp commented Apr 26, 2018

I've got opinions on the rest, preferring to add as few as possible, but I want to go get some more data about what labels are out there and how they're used first. It looks like the proposed list keeps us within https://en.wikipedia.org/wiki/The_Magical_Number_Seven,_Plus_or_Minus_Two so broadly lgtm.

For now, a comment on:

kind/flake (new per #1579)

This isn't new, it's been present in kubernetes/kubernetes, but nowhere else. I was wary of this being noisy, but looking at its use -author:k8s-merge-robot it seems pretty well understood as distinct from kind/bug and distinct from priority/failing-test. So yeah I think I'm in favor of making this org-wide.

@jberkus
Copy link
Contributor Author

jberkus commented Apr 26, 2018

@spiffxp yeah, my criteria for all of the remaining kind/ labels was "can I imagine wanting to run a report on issues/PRs of this type".

@cblecker cblecker added this to Backlog in Contributor Experience via automation May 3, 2018
@cblecker cblecker moved this from Backlog to In Progress in Contributor Experience May 3, 2018
@jberkus
Copy link
Contributor Author

jberkus commented May 3, 2018

Incidentally, I consider this good to go, it's just waiting for conventions to be over so I can carry it out.

@spiffxp
Copy link
Member

spiffxp commented Jul 26, 2018

Deprecated kind labels

  • kind/enhancement (duplicates kind/feature)
    Renames:
  • priority/failing-test to kind/failing-test
  • kind/support to triage/support
    New Labels:
  • kind/flake
    Existing Labels to be added to labels.yaml:
  • kind/cleanup
  • kind/design

OK, this is all done, ref: #1579 and kubernetes/test-infra#8129

As for the rest, many of the labels you reference as unused (in kubernetes/community) are used in kubernetes/kubernetes.

Looking at both, I would suggest (in order of ease):

  • migrate kind/upgrade-test-failure and kind/e2e-test-failure to kind/failing-test
  • (manually) remove kind/mesos-flake and kind/postmortem (kubernetes/kubeadm uses it so I don't want to remove org-wide)
  • chase down why the misc-mungers path-label munger is setting kind/old-docs for PR's that touch k/k/docs, perhaps we can remove it
  • migrate kind/friction to kind/cleanup
  • talk to SIG API Machinery about their use of kind/new-api and kind/api-change, are there better labels or OWNERS files they could be using to support their review process without these additional labels

I'll tackle the first two today. Not sure if/when I'll get to the rest

@spiffxp
Copy link
Member

spiffxp commented Jul 26, 2018

Notified kubernetes-dev@ of manual removal of kind/postmortem and kind/mesos-flake, will remove after 3pm PT today if no objections

https://groups.google.com/forum/#!topic/kubernetes-dev/CGpiuWpp5uE

@cblecker
Copy link
Member

new-api and api-change should actually go to sig-arch. Yes, they actively use the labels.

@jberkus
Copy link
Contributor Author

jberkus commented Jul 26, 2018

API kind labels added to lists above per @cblecker's comment, which I agree with based on 1.11 activity.

Basically, api-change and new-api have specific meanings, and both pass the test of "is this something that I'd want to run a report on". I'd prefer that they both be consolidated into one kind/api-change label,but I'm also not willing to argue with SIG-arch or SIG-api about it.

@spiffxp
Copy link
Member

spiffxp commented Aug 9, 2018

Once the PR I just linked here merges, that leaves us with:

  • chase down why the misc-mungers path-label munger is setting kind/old-docs for PR's that touch k/k/docs, perhaps we can remove it
  • migrate kind/friction to kind/cleanup (I'm slightly unsure if I want to do this now)

@spiffxp
Copy link
Member

spiffxp commented Aug 17, 2018

chase down why the misc-mungers path-label munger is setting kind/old-docs for PR's that touch k/k/docs, perhaps we can remove it

This is no longer user kubernetes/test-infra#9007. Opened #2032 to merge the label into kind/documentation

migrate kind/friction to kind/cleanup (I'm slightly unsure if I want to do this now)

I'm now leaning more toward making this an org-wide label. Adding it puts us at 9 kind/foo labels, which is still within 7 +/- 2, and I think it's a distinct concept in between kind/bug and kind/cleanup, ie:

  • kind/cleanup: it works, but it's a mess
  • kind/friction: it works, but not very well
  • kind/bug: it doesn't work as expected

WDYT?

@jberkus
Copy link
Contributor Author

jberkus commented Aug 17, 2018

I don't buy the logic on keeping kind/friction, because:

  1. Labels are only useful to the extent that we can get contributors to use them consistently. Do you really think that people are going to pick a "distinct concept between bug and cleanup" consistently?

  2. In K/K, the label has been used only 23 times in the history of the repo, and only 3 times in the last year. Further, for all three of the uses in the last year, the issue has another kind/ label in addition to kind/friction, showing that the "friction" label itself is insufficient to categorize the issue (and thus unnecessary).

@spiffxp
Copy link
Member

spiffxp commented Aug 17, 2018

It's been used 28 times across the org. Of those, it was used distinctly 16 times.

The numbers are low enough that I'm sold. I'll propose merging kind/friction into kind/cleanup to k-dev@

@spiffxp
Copy link
Member

spiffxp commented Sep 4, 2018

/close
We're now at 8 distinct org-wide kind/ labels, well within 7 +/- 2, which was my goal. I'm calling this done, please /reopen if I missed any of the original criteria in the description.

I think efforts to further prune one-off kind/ labels are out of scope here. Those labels being...

kind/cluster-api: kubernetes-sigs/cluster-api kubernetes-sigs/cluster-api-provider-gcp
kind/cluster-api-gcp: kubernetes-sigs/cluster-api-provider-gcp
kind/cluster-api-vsphere: kubernetes-sigs/cluster-api
kind/clusterctl: kubernetes-sigs/cluster-api
kind/discuss: kubernetes/dashboard kubernetes/kompose
kind/discussion: kubernetes/dashboard
kind/ecosystem-enablement: kubernetes/contrib
kind/example: kubernetes/contrib
kind/kanarynetes: kubernetes/test-infra
kind/libs: kubernetes/kompose
kind/meta: kubernetes/kompose
kind/refactor: kubernetes/dashboard kubernetes/kubeadm
kind/refactoring: kubernetes/dashboard
kind/repository-infra: kubernetes/kubeadm
kind/test: kubernetes-sigs/gcp-compute-persistent-disk-csi-driver kubernetes/kompose
kind/testing: kubernetes/kompose
kind/tracking-issue: kubernetes/kubeadm
kind/ux: kubernetes/minikube
kind/workaround-removal: kubernetes/contrib
kind/🐶: kubernetes/test-infra
kind/mesos-flake: kubernetes/contrib kubernetes/kubectl
kind/question: kubernetes-sigs/aws-alb-ingress-controller kubernetes-incubator/bootkube
kind/regression: kubernetes-sigs/aws-alb-ingress-controller kubernetes-incubator/bootkube
kind/release: kubernetes-sigs/aws-alb-ingress-controller kubernetes-incubator/bootkube
kind/velocity-improvement: kubernetes/test-infra kubernetes/contrib
kind/postmortem: kubernetes/contrib kubernetes/kubeadm kubernetes/kubectl

@k8s-ci-robot
Copy link
Contributor

@spiffxp: Closing this issue.

In response to this:

/close
We're now at 8 distinct org-wide kind/ labels, well within 7 +/- 2, which was my goal. I'm calling this done, please /reopen if I missed any of the original criteria in the description.

I think efforts to further prune one-off kind/ labels are out of scope here. Those labels being...

kind/cluster-api: kubernetes-sigs/cluster-api kubernetes-sigs/cluster-api-provider-gcp
kind/cluster-api-gcp: kubernetes-sigs/cluster-api-provider-gcp
kind/cluster-api-vsphere: kubernetes-sigs/cluster-api
kind/clusterctl: kubernetes-sigs/cluster-api
kind/discuss: kubernetes/dashboard kubernetes/kompose
kind/discussion: kubernetes/dashboard
kind/ecosystem-enablement: kubernetes/contrib
kind/example: kubernetes/contrib
kind/kanarynetes: kubernetes/test-infra
kind/libs: kubernetes/kompose
kind/meta: kubernetes/kompose
kind/refactor: kubernetes/dashboard kubernetes/kubeadm
kind/refactoring: kubernetes/dashboard
kind/repository-infra: kubernetes/kubeadm
kind/test: kubernetes-sigs/gcp-compute-persistent-disk-csi-driver kubernetes/kompose
kind/testing: kubernetes/kompose
kind/tracking-issue: kubernetes/kubeadm
kind/ux: kubernetes/minikube
kind/workaround-removal: kubernetes/contrib
kind/🐶: kubernetes/test-infra
kind/mesos-flake: kubernetes/contrib kubernetes/kubectl
kind/question: kubernetes-sigs/aws-alb-ingress-controller kubernetes-incubator/bootkube
kind/regression: kubernetes-sigs/aws-alb-ingress-controller kubernetes-incubator/bootkube
kind/release: kubernetes-sigs/aws-alb-ingress-controller kubernetes-incubator/bootkube
kind/velocity-improvement: kubernetes/test-infra kubernetes/contrib
kind/postmortem: kubernetes/contrib kubernetes/kubeadm kubernetes/kubectl

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.

Contributor Experience automation moved this from In Progress to Completed Sep 4, 2018
@jberkus
Copy link
Contributor Author

jberkus commented Sep 4, 2018

All the things in the original issue are done. Great work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig/contributor-experience Categorizes an issue or PR as relevant to SIG Contributor Experience.
Development

No branches or pull requests

5 participants