Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Rethink Labels. #6175

Closed
kianenigma opened this issue May 28, 2020 · 13 comments
Closed

Rethink Labels. #6175

kianenigma opened this issue May 28, 2020 · 13 comments
Assignees
Labels
I6-documentation Documentation needs fixing, improving or augmenting.

Comments

@kianenigma
Copy link
Contributor

Substrate is almost at an important release. While issue management is still something that we need to figure out improvements upon, I think better labels is a good and easy way to start this.

I brought this up a while ago internally as well, yet the conversation did not go anywhere. Here, I will try to rephrase everything given our new recent labels:

Basically, I have two main proposals:

  1. Throw away the entire M group, introduce new ones that show the denote locality.
  2. Make it clear which ones are for pull requests and which ones are for issues. Add a section about labels in our CONTRIBUTING.md and make sure it is being respected.

My current interpretation of labels:

  • A: Only for PRs, shows the progress of the PR.
  • B: Only for PRs, shows the impact zone of the PR. What does it break. Who should be informed. AFAIK we use this to generate release notes.
  • F: Only for issues, shows the broad domain of the issue.
  • P: Only for issues, shows the time-criticality of the issue.
  • Q: Only for issues, shows the difficulty.
  • Z: Only for issues, should technically be used per each closed issues, basically saying how and why it was closed.

and then we have the infamous M which I can barely make any sense thereof:

As far as I can think:

  • Some of these are too fine grained. They pertain to one scope of the project and if we are to allow this, we would have had hundreds of issue labels per crate in substrate. examples being: M6-rpcapi, M7-signer, M8-contracts,
  • Some seem totally out of scope: M8-dapp, M7-ui.
  • I think too many of them are related to CI/Maintenance: M9-deploy ,M2-installer, M2-config, M1-ci, M0-build M5-binaries,
  • M3-docs is duplicate and we have a better label in F group for docs.
  • and finally M4-Core is assigned to what seems to me a random set of non-runtime issues. The term core itself used to be folder name in substrate that no longer exists.

I think the best replacement for M is to have add a group of labels that show the locality of the issue. For example now we have a label for refactoring. But there's no way to distinguish between where this refactor might happen. Well.. except this M4-core for all the no-runtime stuff.

Perhaps we can come up with a better grouping that explains locality

  • Runtime/Frame
  • Crypto
  • Storage/State/Overlay
  • Consensus/Authoring (not to be confused with F0-Consensus)
  • Genesis/ChainSpec
  • Network
  • CLI/RunCommands
  • (and instead of all the above) CI/Maintenance
  • ...

Probably some of the team leads or elder devs who have a better big picture of the project can come up with the above list, or even argue that it is pointless and we should not have such locality labels. Heck, maybe we should reformat M to actually be fine grained stuff, and have many of them. Even so, this must be documented somewhere. For example, I would love to have an issue that groups all the issues related to weights. They are a recent effort that will probably not fit into a coarse grained grouping, yet I don't know if I can/should make one etc. One way or another though, I think the M should be cleaned. Either replaced, or it should be clarified why it is there and what is its purpose.

All in all, I know that this sounds like a pointless effort at the moment, but I think it is a right step and we need to start taking better care of our issue at some point. We currently have two issues with.. issues:

  1. There are simply too many of them for us to even keep track of.
  2. Even for those that we do keep track of (i.e. dev X starts a conversation there and ...) we don't have proper labels and they are a bit of a mess.

At least, if have proper issues, with well defined rules (probably in our CONTRIBUTING.md) of how to assign them (i.e. I think the Z is totally underrated and should be used more) we can solve at least one part of the problem.

@kianenigma kianenigma added I6-documentation Documentation needs fixing, improving or augmenting. J2-unconfirmed Issue might be valid, but it’s not yet known. labels May 28, 2020
@kianenigma
Copy link
Contributor Author

kianenigma commented Jun 5, 2020

Based on the labels that we already have I propose the following automated process as well for the issues and labels. This is based on the fact that we now have teams internally within Parity.

  • We remove the M label and each team will have at least one, or more, label for its domain of work. This solves the dilemma above about domain-specific labels. I call this T. For example T0-Runtime, T1-Networking, .. and sub groups such as T0-Runtime-weights that can live for a while.

Then we have the following categories for issues:
1- F: type of work: bug fix, consensus critical, refactor, documentation, etc.
2- P: time
3- Q: difficulty and mentoring
4- T: team and domain that the issue belongs to it. This one is mandatory and each issue must either have this, or Z0-unconfirmed

Then, we can have a bot that does the following:

  • If an issue is created by a non-parity member, it is instantly marked as Z0-unconfrmed until a core dev verifies the issue and removes it. Upon removal, it should be labeled with T or optionally more labels.

    • If the issue is being closed, the rest of the a Z should be used to say why it is being closed.
  • For any issue created by a parity member, it MUST have at least a T label.

@s3krit s3krit self-assigned this Jun 5, 2020
@s3krit
Copy link
Contributor

s3krit commented Jun 5, 2020

Agree with everything here - labels and issue handling could do with some love. I'll add a Github Action for auto-assigning issues submitted by people outside of the organization the Z0-unconfirmed label, this seems pretty straightforward. This will at least be a first baby-step towards better issue automation.

Additional automation could be done, but not without a little more information available. For example, if someone from team X confirms the issue, does that issue get auto-assigned to the team that dev is in? To do that would require more granular team definitions in the Paritytech org. Also I'm not sure we can enforce the presence of a label on issue creation. At a minimum, I suppose we could have a bot comment on the issue informing the dev to assign an appropriate label.

@kianenigma
Copy link
Contributor Author

Additional automation could be done, but not without a little more information available. For example, if someone from team X confirms the issue, does that issue get auto-assigned to the team that dev is in?

That would be too hard I think. Whoever reads the issue from team X is way better aware of which team the issue belongs to.

Also I'm not sure we can enforce the presence of a label on issue creation. At a minimum, I suppose we could have a bot comment on the issue informing the dev to assign an appropriate label.

Yes indeed, I don't think it can be prevented either, but we can have a bot that nudges at you if you don't :)

@athei
Copy link
Member

athei commented Jun 10, 2020

I would also add that I have to idea which B label to add to my PR. Do I have to add at least one? Can I have multiple ones? The descriptions of those labels do not make it clear to me which of those I have to apply.

@kianenigma
Copy link
Contributor Author

update from @gavofyork:

Public service notice: PRs must now be labeled with one A* label, one B* label and one C* label. They should also have the project specified.

@kianenigma
Copy link
Contributor Author

One step forward #6333

@gavofyork
Copy link
Member

gavofyork commented Jun 12, 2020

PRs

  • A-* Pull request status. @s3krit: Automation should ensure this is kept in sync with GH's native review system. ONE REQUIRED.
  • B-* Changelog and/or Runtime-upgrade post composition markers. Automation uses this. ONE REQUIRED.
  • C-* Release notes release-priority markers. Automation uses this. EXACTLY ONE REQUIRED.
  • D-* More general tags on the PR denoting various implications and requirements.

Issues

  • I-* Issue severity and type. EXACTLY ONE REQUIRED.
  • P-* Issue priority. AT MOST ONE ALLOWED.
  • Q-* Issue difficulty. AT MOST ONE ALLOWED.
  • Z-* More general tags on the issue, denoting context and resolution.

I have removed the M-* class as they have in any case been superseded by GH's Project functionality.

@kianenigma
Copy link
Contributor Author

This is progressing fast and I think the core problems are resolved. @s3krit please re open if you want this issue as tracking for the CI implementation, otherwise it is done IMO.

@kianenigma kianenigma removed the J2-unconfirmed Issue might be valid, but it’s not yet known. label Jun 23, 2020
@kianenigma
Copy link
Contributor Author

I would like to propose re-creating the M group but this time with sensible project scopes. For example, I can bring quite a few issues under one roof with the label M0-NPoS 💸 and it should be helpful to have.

@s3krit
Copy link
Contributor

s3krit commented Aug 13, 2020

Would these be for classifying both PRs and issues?

@kianenigma
Copy link
Contributor Author

Just issues, e.g. https://github.com/paritytech/substrate/issues/6835.

@s3krit
Copy link
Contributor

s3krit commented Aug 18, 2020

Cool. What labels should we add @kianenigma ?

@kianenigma
Copy link
Contributor Author

I settled for adding a new project column. Thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
I6-documentation Documentation needs fixing, improving or augmenting.
Projects
None yet
Development

No branches or pull requests

4 participants