Skip to content

[core] Add CI check that the PR has label#849

Merged
oliviertassinari merged 2 commits into
mui:masterfrom
oliviertassinari:ensure-pr-has-label
Oct 19, 2022
Merged

[core] Add CI check that the PR has label#849
oliviertassinari merged 2 commits into
mui:masterfrom
oliviertassinari:ensure-pr-has-label

Conversation

@oliviertassinari

@oliviertassinari oliviertassinari commented Aug 24, 2022

Copy link
Copy Markdown
Member

This is probably too early to enforce at least one GitHub label on PRs but part of https://www.notion.so/mui-org/Retrospective-W32-33-cfd4332ad6a54f4593b453a8ec0bf91f#edf81bb00b504c8abd5b970aff98a77a. I believe that we eventually want to get there. Enforcing labels could solve:

  1. Scaling reviews. It's what we use in https://www.notion.so/mui-org/0c8156b0b1634ea5bfd903dd005ec1cf?v=b9e94cc3ea554e668e7c54d585910569
  2. Guiding high-level resource allocations. https://www.notion.so/mui-org/KPIs-1ce9658b85ce4628a2a2ed2ae74ff69c#06950ec6688840549cfc9429635f4298
  3. Finding past work on the problems faster (in Material UI, it's often impossible without labels)

I have copied mui/material-ui#32886

@oliviertassinari oliviertassinari added the internal Behind-the-scenes enhancement. Formerly called “core”. label Aug 24, 2022
@bytasv

bytasv commented Aug 25, 2022

Copy link
Copy Markdown
Contributor

Are there any downsides if we try to enforce at least one label? Worse case scenario we can add just any label to make PR pass and document why we could use other meaningful labels, or if we need to create new ones 🤔

@oliviertassinari

oliviertassinari commented Aug 25, 2022

Copy link
Copy Markdown
Member Author

Are there any downsides if we try to enforce at least one label?

It's a bit more friction, the GitHub Action takes ~20s to run, a time you have to wait before you can merge your PR if a label is missing.

As a general rule, I would personally be in favor of us feeling pain around the lack of labels before merging this.

@bytasv

bytasv commented Aug 25, 2022

Copy link
Copy Markdown
Contributor

It's a bit more friction, the GitHub Action takes ~20s to run, a time you have to wait before you can merge your PR if a label is missing.

Don't we need to wait for like 10-20 minutes before circle CI jobs complete? 🤔

I was also thinking if we could run GH action but make it optional in order to merge PR? That way it would introduce little bit of noise to nudge us to start using labels, but would not block merging of PRs?

@Janpot

Janpot commented Aug 29, 2022

Copy link
Copy Markdown
Member

I would personally be in favor of us feeling pain around the lack of labels before merging this.

I'm fine with merging, but this resonates with me. Without a clear purpose, I'm not sure we would be investing much time in the quality of our labeling.

@apedroferreira

Copy link
Copy Markdown
Collaborator

Maybe something like a very short reminder in the PR template would work for now?
Even though if it only takes 20s for the CI to report missing labels it sounds fine too.

@oliviertassinari
oliviertassinari marked this pull request as ready for review October 12, 2022 22:27
Comment thread .github/workflows/check-if-pr-has-label.yml
@oliviertassinari oliviertassinari removed the internal Behind-the-scenes enhancement. Formerly called “core”. label Oct 12, 2022
@oliviertassinari

Copy link
Copy Markdown
Member Author

I came back today to this PR after seeing some of the regressions, which made me think that having some metrics could help us identify if we write enough tests or not, identify if we focus enough/too much on bugs. Forcing at least one label could cover:

@oliviertassinari
oliviertassinari force-pushed the ensure-pr-has-label branch 4 times, most recently from eaac02c to 573b3c9 Compare October 12, 2022 22:49
@oliviertassinari oliviertassinari added the internal Behind-the-scenes enhancement. Formerly called “core”. label Oct 12, 2022
@oliviertassinari oliviertassinari added internal Behind-the-scenes enhancement. Formerly called “core”. and removed internal Behind-the-scenes enhancement. Formerly called “core”. labels Oct 12, 2022
oliviertassinari added a commit to step-security-bot/mui-x that referenced this pull request Oct 12, 2022
Signed-off-by: Olivier Tassinari <olivier.tassinari@gmail.com>
@Janpot

Janpot commented Oct 13, 2022

Copy link
Copy Markdown
Member

I think we should label in function of what we want to measure. We're currently labelling bugs and regressions.

some metrics could help us identify if we write enough tests or not, identify if we focus enough/too much on bugs.

If we want to effectively measure these two things, which extra labelling would be required?

@oliviertassinari

oliviertassinari commented Oct 13, 2022

Copy link
Copy Markdown
Member Author

in function of what we want to measure.

@Janpot At our scale, a few ideas of concrete problems that it could solve:

  • To find older work. We are getting close to #1000. We might want to find discussions on older work, without the labels to filter out issues/PRs, good luck.
  • To provide clarity on what the issue/PR is about. Sometimes simply choosing between, a bug, a new feature, support: question, and docs, can require a very deep understanding of the pain point.
  • To delegate the area of ownership, with broad product scope labels, we could start to prepare when we reach the point where the is too much activity on the repository for a single person to be able to see everything.
  • (what I mentioned in [core] Add CI check that the PR has label #849 (comment)) Maybe to track regressions, I'm not sure, maybe it's helpful as a broad feedback loop for us to: if there are too many then it means that we are moving without testing enough.

@Janpot

Janpot commented Oct 14, 2022

Copy link
Copy Markdown
Member

To provide clarity on what the issue/PR is about. Sometimes simply choosing between, a bug, a new feature, support: question, and docs, can require a very deep understanding of the pain point.

Yes, those labels I agree with.

To delegate the area of ownership, with broad product scope labels, we could start to prepare when we reach the point where the is too much activity on the repository for a single person to be able to see everything.

I'm not sure I currently see clear lines where we can determine ownership. I was hoping for the future to do that more along the lines of CE, EE, cloud, docs. The current labels like "feature: queries" don't feel very useful to me. e.g. I change something that affects all queries, does it go in "core" or in "feature: queries"?

@oliviertassinari

Copy link
Copy Markdown
Member Author

The current labels like "feature: queries" don't feel very useful to me.

We can remove these, until we see a clear pain point it solves. At this point, my main assumption is that the feature labels can help to find patterns of bugs, find duplicates, but if the volume is low enough to be able to see all the new issues, then yeah, agree, it's overkill at our size.

@Janpot

Janpot commented Oct 15, 2022

Copy link
Copy Markdown
Member

Ok, sounds good. So in terms of which labels have to be applied, how about

  • core
  • docs

to define area of ownership (We can add more labels when necessary), optionally accompanied by

  • bug
  • regression
  • feature
  • enhancement
  • ...?

to define the nature of the PR?

Question: With this github action, can we define which labels to pick from? e.g. PR must have one blue label (core/docs) and one yellow label (bug/regression/..)

@oliviertassinari

oliviertassinari commented Oct 15, 2022

Copy link
Copy Markdown
Member Author

Question: With this github action, can we define which labels to pick from? e.g. PR must have one blue label (core/docs) and one yellow label (bug/regression/..)

This GitHub Action doesn't support it. @mnajdova wrote it to help us find cases where we forget about applying any labels.

The problem we had was sometimes we forget about adding the labels altogether. It assumes that once we remember the labels, we then do it "correctly". It's not perfect, but honestly, I don't see the need to be more granular.

oliviertassinari added a commit to step-security-bot/mui-x that referenced this pull request Oct 15, 2022
Signed-off-by: Olivier Tassinari <olivier.tassinari@gmail.com>
oliviertassinari added a commit to step-security-bot/mui-x that referenced this pull request Oct 15, 2022
Signed-off-by: Olivier Tassinari <olivier.tassinari@gmail.com>

@Janpot Janpot left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ok to merge for me. Ideally it's documented somewhere which labels are expected to keep it from becoming ambiguous.

Does the labels option allow for specifying which labels are expected?

@oliviertassinari

Copy link
Copy Markdown
Member Author

Does the labels option allow for specifying which labels are expected?

Based on https://github.com/mheap/github-action-required-labels#examples, yes, I think that it could be possible.

@github-actions github-actions Bot added the PR: out-of-date The pull request has merge conflicts and can't be merged. label Oct 15, 2022
@github-actions github-actions Bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged. label Oct 19, 2022
@oliviertassinari
oliviertassinari merged commit cebc5ee into mui:master Oct 19, 2022
@oliviertassinari
oliviertassinari deleted the ensure-pr-has-label branch October 19, 2022 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

internal Behind-the-scenes enhancement. Formerly called “core”.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants