DNM: Proposal for unifying and improving labels in TiDB, TiKV, and PD repos #11729

nrc · 2019-08-13T07:14:49Z

For discussion only. See the document for description, etc: rendered

PTAL everyone!

Leave comments by reviewing

For discussion only

winkyao · 2019-08-13T07:21:47Z

IMHO, category/label is better because it's more readable.

labels.md

zz-jason · 2019-08-13T07:30:29Z

labels.md

+errmsg                          #a7c938     T: Error message                    #1d76db
+for new contributors            #c2e0c6     D: Mentor                           #0e8a16
+help wanted                     #159818     S: HelpWanted                       #e6e6e6
+needs-cherry-pick-2.1           #000000     T: CherryPick-2.1                   #1d76db


This label means the PR needs to be cherry-picked to the 2.1 release branch.

zz-jason · 2019-08-13T07:31:54Z

labels.md

+status/future                   #fbca04     ?                                   
+type/1.0 cherry-pick            #c2e0c6     *                                   
+type/2.0 cherry-pick            #99d0ef     *                                   
+type/2.1 cherry-pick            #9bfff5     * <T: CherryPick-2.1>               


this label means the PR is a cherry-pick on the 2.1 release-branch.

What is this used for? I.e., why does it matter if a PR has been cherry-picked?

ping @zz-jason question ^

winkyao

Rest LGTM

winkyao

.

labels.md

zz-jason · 2019-08-13T07:43:19Z

labels.md

+status/DNM                      #b60205     S: DNM                              #e6e6e6
+status/LGT1                     #d4c5f9     * <S: Waiting on review>            
+status/LGT2                     #5319e7     * <S: Waiting on review>            
+status/LGT3                     #330099     * <S: Waiting on review>            


Typically, we use these kind of labels to tell whether we can approve the PR after we comment a LGTM on the PR. If the PR is labeled with LGT1, then we can approve the PR when we comment another LGTM.

Are these useful? It seems to me, you can see this from looking at the GitHub reviews (which I guess didn't exist when these labels were created?). And I think a reviewer or author no longer has to merge/approve the PR since the bot should do that automatically once a PR has enough reviews and passing CI.

labels.md

nrc · 2019-08-13T20:47:28Z

I've updated to take comments into account and make spacing and capitalisation more consistent. I haven't changed the naming scheme yet.

Do others prefer category/label too?

shenli · 2019-08-13T21:16:43Z

I've updated to take comments into account and make spacing and capitalisation more consistent. I haven't changed the naming scheme yet.

Do others prefer category/label too?

I prefer category/lable. Kubernetes follows this way. See: https://github.com/kubernetes/kubernetes/issues

nrc · 2019-08-13T22:33:28Z

Now with category/label

labels.md

Hoverbear

Generally LGTM, this feels very familiar from all the times I browsed rust repos. :)

shenli · 2019-08-14T08:20:18Z

The labels look good to me. But I have another question. Where should we put this document? In some dir of the TiDB repo or in the community repo?

nrc · 2019-08-14T09:35:51Z

The labels look good to me. But I have another question. Where should we put this document? In some dir of the TiDB repo or in the community repo?

I wasn't planning to keep this document, just make the changes to the labels and close this PR.Do you want to keep it?

Hoverbear · 2019-08-14T18:21:09Z

I'd like to see the TiKV portion go to our RFCs repo. :) Let's get two approvals form the TiKV maintainers first so we can be sure to merge it without change.

cc @BusyJay @zhangjinpeng1987 @siddontang @sunxiaoguang PTAL and let us know if you are willing to merge this as-is as a TiKV RFC.

nrc · 2019-08-15T04:39:35Z

ping @zz-jason I've addressed some of your comments in the labels doc and have replied to other comments inline. PTAL

labels.md

nrc · 2019-08-19T06:18:56Z

re-ping @zz-jason are you satisfied with the answers to your questions?

breezewish · 2019-08-19T09:39:35Z

labels.md

+## TiKV
+
+```
+C: Build                        #d1fad7     component/build                     #d1fad7


Looks like these two conflicts to each other: "We use a C: label scheme for all repos" "Current labels and colours are on the left, proposed are on the right"

breezewish · 2019-08-19T09:40:12Z

labels.md

+C: Txn                          #d1fad7     component/transactions              #d1fad7
+C: Util                         #d1fad7     component/util                      #d1fad7
+C: gRPC                         #d1fad7     component/gRPC                      #d1fad7
+D: Easy                         #0e8a16     difficulty/easy                     #0e8a16


Note that we previously used labels like "Component: Copr". However it is too long so changed to "C: Copr".

Too long means there is a length limit on GitHub or it didn't look good?

It didn't look good, since usually there will be many labels and long labels makes the issue / PR list looks disheveled. Also short label is less distractive:

TiDB PR list

vs TiKV PR list

labels.md

breezewish · 2019-08-19T09:43:09Z

labels.md

+                                            component/bench                     #d1fad7
+C: TiKV-Client                  #d1fad7     component/TiKV client               #d1fad7
+C: TiKV-Ctl                     #d1fad7     component/TiKV ctl                  #d1fad7
+C: Titan                        #d1fad7     component/Titan                     #d1fad7


Looks like we can merge C: RocksDB and C: Titan into C: Engine

siddontang · 2019-08-21T15:11:35Z

@Hoverbear

I am not sure whether putting it in RFC is good or not. But to unify the labels for all Repos, I think we should use a place to put it.

siddontang · 2019-08-21T15:15:01Z

Thanks, @nrc

here I just see the color number like #0e8a16, but what does it really look like? How can we know it?

siddontang · 2019-08-21T15:16:24Z

labels.md

@@ -0,0 +1,220 @@
+# Goals
+
+* synchronise labels between repos


do we need to capitalize the first letter?
/cc @dcalvin

We should - but it does not matter here, as the point of this document is to finalize the labels.

labels.md

nrc · 2019-08-23T04:22:53Z

here I just see the color number like #0e8a16, but what does it really look like? How can we know it?

Here's what each label looks like. All labels in the same category would have the same colours.

nrc · 2019-09-02T21:04:56Z

@siddontang what do you think of the colours in #11729 (comment)?

@zz-jason are you happy with the answers to your questions inline?

zz-jason · 2019-09-03T08:54:59Z

LGTM

breezewish

What's the status of #11729 (comment) ?

nrc · 2019-09-05T02:53:02Z

What's the status of #11729 (comment) ?

@winkyao @shenli and @siddontang prefer category/label, @breeswish prefers C: label, I don't think anyone else has voiced an opinion.

siddontang

LGTM

/cc @zhouqiang-cl, please pay attention to this PR, which may affect CI, benchbot, etc.

zhouqiang-cl · 2019-09-09T04:20:18Z

ok

zz-jason · 2019-09-10T04:36:05Z

What's the status of #11729 (comment) ?

@winkyao @shenli and @siddontang prefer category/label, @breeswish prefers C: label, I don't think anyone else has voiced an opinion.

I vote for category/label

breezewish · 2019-09-10T08:13:11Z

labels.md

+
+```
+C: Build                        #d1fad7     component/build                     #d1fad7
+C: Build-Time                   #d1fad7     component/build time                #d1fad7


look like build time is a subset of build

breezewish · 2019-09-10T08:13:36Z

labels.md

+C: Copr                         #d1fad7     component/coprocessor               #d1fad7
+C: Doc                          #d1fad7     component/docs                      #d1fad7
+C: PD-Client                    #d1fad7     component/PD client                 #d1fad7
+C: Perf                         #d1fad7     component/perf                      #d1fad7


since now full names are preferred then it should be component/performance?

breezewish · 2019-09-10T08:13:57Z

labels.md

+C: Doc                          #d1fad7     component/docs                      #d1fad7
+C: PD-Client                    #d1fad7     component/PD client                 #d1fad7
+C: Perf                         #d1fad7     component/perf                      #d1fad7
+C: Raft                         #d1fad7     component/raft                      #d1fad7


It should be Raft instead of raft?

breezewish · 2019-09-10T08:14:08Z

labels.md

+C: Test/Bench                   #d1fad7     component/test                      #d1fad7
+                                            component/bench                     #d1fad7
+C: TiKV-Client                  #d1fad7     component/TiKV client               #d1fad7
+C: TiKV-Ctl                     #d1fad7     component/TiKV ctl                  #d1fad7


TiKV control?

breezewish · 2019-09-10T08:14:37Z

labels.md

+C: gRPC                         #d1fad7     component/gRPC                      #d1fad7
+D: Easy                         #0e8a16     difficulty/easy                     #0e8a16
+D: Medium                       #f4b169     difficulty/medium                   #0e8a16
+D: Mentor                       #31c639     difficulty/mentor                   #0e8a16


BTW I'm curious what is a "mentor" difficulty XD

can we remove this label?

breezewish · 2019-09-10T08:15:23Z

labels.md

+S: CanMerge                     #4be524     status/bot merge                    #e6e6e6
+S: DNM                          #DDDDDD     status/DNM                          #e6e6e6
+S: Discussion                   #fbca04     type/discussion                     #1d76db
+S: Duplicate                    #dddddd     status/closed dup                   #e6e6e6


It should be the full name "status/closed for duplicate"?

breezewish · 2019-09-10T08:16:12Z

labels.md

+P: Low                          #eeee00     priority/low                        #eb6420
+P: Release-blocker              #f25c8e     priority/blocker                    #eb6420
+S: BotClose                     #c6054c     status/bot close                    #e6e6e6
+S: CanMerge                     #4be524     status/bot merge                    #e6e6e6


bot close is closed by bot but bot merge is not merged by bot, so I think they should not looks to be similar at least.

breezewish · 2019-09-10T08:17:49Z

labels.md

+S: Duplicate                    #dddddd     status/closed dup                   #e6e6e6
+S: HelpWanted                   #fbca04     status/help wanted                  #e6e6e6
+S: Invalid                      #dddddd     status/closed invalid               #e6e6e6
+S: LGT1                         #66d7ee     status/waiting on review            #e6e6e6


Looks like normal PRs should mostly be in a "waiting on review" status and then this label does not have much meaning?

breezewish · 2019-09-10T08:18:46Z

labels.md

+T: CherryPick                   #1d76db     * <type/cherry pick 2.1 or type/cherry pick 3.0>
+T: Contributor ⭐️               #1d76db     type/contributor ⭐️                 #1d76db
+T: Enhancement                  #1d76db     type/enhancement                    #1d76db
+T: NeedCherryPick-2.1           #333333     type/cherry pick 2.1                #1d76db


T: NeedCherryPick is not the same as cheery pick. NeedCherryPick is a hint for bot which means bot will create a cherry pick PR for it later and this PR is usually NOT a cheery pick, while T: CherryPick means this PR is a cheery pick.

breezewish · 2019-09-10T08:20:16Z

labels.md

+S: Waiting                      #DDDDDD     status/waiting on author            #e6e6e6
+T: Bug                          #d93f0b     type/bug                            #1d76db
+T: BugFix                       #1d76db     type/bug fix                        #1d76db
+T: CHANGELOG                    #006b75     type/CHANGELOG                      #1d76db


This label is rarely used and I think it can be removed.

breezewish · 2019-09-10T08:24:04Z

labels.md

+D: Mentor                       #31c639     difficulty/mentor                   #0e8a16
+P: Critical                     #ed0000     priority/high                       #eb6420
+P: High                         #ed8888     priority/medium                     #eb6420
+P: Low                          #eeee00     priority/low                        #eb6420


I don't think it's appropriate for different priorities to have the same color.

breezewish · 2019-09-10T08:25:44Z

labels.md

+S: Discussion                   #fbca04     type/discussion                     #1d76db
+S: Duplicate                    #dddddd     status/closed dup                   #e6e6e6
+S: HelpWanted                   #fbca04     status/help wanted                  #e6e6e6
+S: Invalid                      #dddddd     status/closed invalid               #e6e6e6


In TiKV, Gray status is specifically marked for those PRs that is no need to take a look, e.g. invalid, working in progress, waiting for the author, do not merge, etc. Other status has different (i.e. vivid) colors that means it deserves a look. So it may be not appropriate for all status to have the same gray color.

breezewish · 2019-09-10T09:01:24Z

Top most GitHub engineering projects, order by stars, are following these styles:

Project	Stars	#Rank	Style
freeCodeCamp	304K	1	status: need update
vue	146K	3	need update
react	135K	4	Status: Need Update
bootstrap	135K	5	need update
tensorflow	132K	6	status: need update
oh-my-zsh	94K	10	Status: update
d3	86K	15	(no labels)
vscode	81K	16	need update
react-native	80K	17	Status: Need Update
electron	76K	20	status/need-update
create-react-app	71K	23	status: need update
node	63K	25	need update
go	62K	26	NeedUpdate
animate.css	61K	27	need update
angular	59K	29	status: need update
kubernetes	56K	31	status/need-update

Maybe neither S: Need Update nor status/need update is a good style, considering that in top 30 no repo is using that style. I suggest it to be Status: Need Update style because it's the most popular style for labels and has a category prefix.

zz-jason · 2019-12-17T12:34:04Z

labels.md

+status/all tests passed         #2cbe4e     * <status/bot merge>                    
+status/can merge                #2cbe4e     status/bot merge                    #e6e6e6
+status/future                   #fbca04     ?                                   
+type/1.0 cherry-pick            #c2e0c6     *                                   


By using labels like type/cherry pick 2.1 to identify whether a PR on the master branch needs to be cherry-picked to release 2.1, we also need labels to identify which branch the PR is submitted on.

A typical use case is we need to check whether there still some un-merged PRs on a release branch, such as release-2.1, before releasing a new version on that release branch.

How about using the following labels to identify which branch the PR is submitted on:

branch/release-2.0 branch/release-2.1 branch/release-3.0

nrc · 2020-02-27T21:14:29Z

Update @zhouqiang-cl has made (roughly) these changes to the TiKV repo.

zz-jason · 2020-04-22T11:52:44Z

Hi @nrc, since all the repos are following this proposal, can we close or merge this PR?

DNM

2b90d38

For discussion only

nrc added type/question The issue belongs to a question. proposal status/DNM labels Aug 13, 2019

nrc requested review from siddontang and shenli August 13, 2019 07:14

This comment has been minimized.

Sign in to view

zhouqiang-cl reviewed Aug 13, 2019

View reviewed changes

labels.md Outdated Show resolved Hide resolved

zz-jason reviewed Aug 13, 2019

View reviewed changes

winkyao reviewed Aug 13, 2019

View reviewed changes

labels.md Outdated Show resolved Hide resolved

zz-jason reviewed Aug 13, 2019

View reviewed changes

labels.md Show resolved Hide resolved

winkyao reviewed Aug 13, 2019

View reviewed changes

labels.md Show resolved Hide resolved

zz-jason reviewed Aug 13, 2019

View reviewed changes

Updated

ac79dd7

This comment has been minimized.

Sign in to view

Use category/label scheme

63bfbd3

Hoverbear reviewed Aug 14, 2019

View reviewed changes

labels.md Outdated Show resolved Hide resolved

Hoverbear reviewed Aug 14, 2019

View reviewed changes

use lower case for label's initial letter

cc44157

sunxiaoguang reviewed Aug 15, 2019

View reviewed changes

labels.md Show resolved Hide resolved

breezewish reviewed Aug 19, 2019

View reviewed changes

labels.md Show resolved Hide resolved

breezewish reviewed Aug 19, 2019

View reviewed changes

siddontang reviewed Aug 21, 2019

View reviewed changes

labels.md Show resolved Hide resolved

Merge branch 'master' into labels

294580e

breezewish reviewed Sep 5, 2019

View reviewed changes

siddontang approved these changes Sep 9, 2019

View reviewed changes

breezewish reviewed Sep 10, 2019

View reviewed changes

zz-jason reviewed Dec 17, 2019

View reviewed changes

nrc closed this Apr 22, 2020

zhouqiang-cl deleted the labels branch December 24, 2020 03:54

DNM: Proposal for unifying and improving labels in TiDB, TiKV, and PD repos #11729

DNM: Proposal for unifying and improving labels in TiDB, TiKV, and PD repos #11729

Conversation

nrc commented Aug 13, 2019 • edited Loading

This comment has been minimized.

winkyao commented Aug 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

winkyao left a comment

Choose a reason for hiding this comment

winkyao left a comment • edited Loading

Choose a reason for hiding this comment

zz-jason Aug 13, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

nrc commented Aug 13, 2019

shenli commented Aug 13, 2019

nrc commented Aug 13, 2019

Hoverbear left a comment

Choose a reason for hiding this comment

shenli commented Aug 14, 2019

nrc commented Aug 14, 2019

Hoverbear commented Aug 14, 2019

nrc commented Aug 15, 2019

nrc commented Aug 19, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

breezewish Aug 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siddontang commented Aug 21, 2019

siddontang commented Aug 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nrc commented Aug 23, 2019

nrc commented Sep 2, 2019

zz-jason commented Sep 3, 2019

breezewish left a comment

Choose a reason for hiding this comment

nrc commented Sep 5, 2019

siddontang left a comment

Choose a reason for hiding this comment

zhouqiang-cl commented Sep 9, 2019

zz-jason commented Sep 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

breezewish Sep 10, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

breezewish Sep 10, 2019 • edited Loading

Choose a reason for hiding this comment

breezewish commented Sep 10, 2019 • edited Loading

Choose a reason for hiding this comment

nrc commented Feb 27, 2020

zz-jason commented Apr 22, 2020

nrc commented Aug 13, 2019 •

edited

Loading

winkyao left a comment •

edited

Loading

zz-jason Aug 13, 2019 •

edited

Loading

breezewish Aug 21, 2019 •

edited

Loading

breezewish Sep 10, 2019 •

edited

Loading

breezewish Sep 10, 2019 •

edited

Loading

breezewish commented Sep 10, 2019 •

edited

Loading