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

[core] Specify code owners #33995

Merged
merged 3 commits into from Aug 24, 2022
Merged

[core] Specify code owners #33995

merged 3 commits into from Aug 24, 2022

Conversation

michaldudak
Copy link
Member

@michaldudak michaldudak commented Aug 19, 2022

@michaldudak michaldudak added the core Infrastructure work going on behind the scenes label Aug 19, 2022
@michaldudak michaldudak requested a review from a team August 19, 2022 16:39
@mui-bot
Copy link

mui-bot commented Aug 19, 2022

No bundle size changes

Generated by 🚫 dangerJS against ff695ca

Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

I'm curious to see how this one will scale, happy we are trying something out.

I suspect that it will be an intermediary step toward a custom-built GitHub action. Because I believe this needs to be powered by GitHub labels. I have found a few that we could use as inspiration:

I also had a quick play with the Notion's API, to see if we could access the product owners' database in a secure way (the scope of the following secret is very limited, read-only and only one database) inside a GitHub action. It seems that we can, relying on the GitHub Labels:

import { Client } from '@notionhq/client';
import fetch from 'node-fetch';

const notion = new Client({ auth: 'secret_uxZfsqctuAKJ6BUompv1uQvH50mzFH72EJ2GYXhtTdg' });

// Workaround https://github.com/makenotion/notion-sdk-js/issues/334
const fetchRowData = async ({ id }) => {
  const response = await fetch(`https://api.notion.com/v1/pages/${id}`, {
    headers: {
      Authorization: `Bearer secret_uxZfsqctuAKJ6BUompv1uQvH50mzFH72EJ2GYXhtTdg`,
      "Notion-Version": "2022-02-22",
    },
  });

  return response.json();
};

(async () => {
  try {
    const { results } = await notion.databases.query({
      database_id: '0c8156b0-b163-4ea5-bfd9-03dd005ec1cf',
    });

    const databaseRowPromises = results.map(async ({ id }) => {
      const {properties} = await fetchRowData({ id });
      return properties;
    });

    const data = await Promise.all(databaseRowPromises);

    console.log('data', data.map(item => ({
      label: item['GitHub label'].multi_select[0]?.name,
      scope: item.Scope.title[0].plain_text,
    })))
  } catch (error) {
    console.log('err',error)
  }
})();

Screenshot 2022-08-20 at 14 07 16

But I suspect that we shouldn't use it, it could become more complex, not simpler than to start with a Tier 1 support agent who does this manually. Because it's done manually, we will learn the rules that we need.

cc @DanailH as I believe wanted to solve a problem related to this one.

.github/CODEOWNERS Outdated Show resolved Hide resolved
.github/CODEOWNERS Outdated Show resolved Hide resolved
/packages/mui-system/ @mnajdova @siriwatknp

# Components - Inputs
/packages/mui-material/src/Autocomplete/ @michaldudak @mnajdova
Copy link
Member

Choose a reason for hiding this comment

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

Should we include here the components from mui-base too?

I would have structured this a bit differently, keeping in mind the long term goal where base would be used in both Joy UI and Material UI, by listing here the components from /packages/mui-base/*, and leaving everything in Joy UI and Material UI to the owners of those packages, considering the implementation there is mostly related to the design system. But as this moment this is not the case happy to start with listing both Material UI and MUI Base components.

Copy link
Member Author

Choose a reason for hiding this comment

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

At some point in the future, yes. However, as we're still at a stage where the API shape may change, I prefer to take ownership of Base as a whole so that I can keep an eye on consistency.

Copy link
Member

Choose a reason for hiding this comment

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

In this case, it would probably mean that we need both the component owner + you for reviewing @mui/base related changes. I guess this could work 👍

BTW, I am curious, as more specific paths are assign to most of the packages/mui-material/* paths, when are we going to trigger the packages/mui-material/ owners?

Copy link
Member

Choose a reason for hiding this comment

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

Should we include here the components from mui-base too?

Strong 👍 for this one and for Joy UI too. It's the best opportunity for people that are exposed to real life challenged surfaced on Material UI to transfer this knowledge to MUI Base and Joy UI as soon as possible (code changes that are delayed are a lot more painful to make happen). For me the problem I'm concerned about is not so much the API for developers but more the features, for end-users.

Copy link
Member Author

Choose a reason for hiding this comment

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

As we discussed at the meeting on Monday, I added just the Base directories.

Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
@DanailH
Copy link
Member

DanailH commented Aug 22, 2022

cc @DanailH as I believe wanted to solve a problem related to this one.

Yes, I was wondering if we can do the same in @mui/x 🤔. The problem there is that we can add owners for the hooks files that define specific features but it's rare that a feature is only contained within 1 hooks file.

On another note - there was an idea to automatically tag code owners based on what label was selected for the PR or issue. I haven't had time to explore this further but it can be more scalable than the code owners file, at least for MUI X.

Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

Ok, it looks good to me. We should just make sure to keep this up to date as we add more components :) Thanks for taking care of this @michaldudak

@michaldudak michaldudak merged commit bb9075c into mui:master Aug 24, 2022
@michaldudak michaldudak deleted the codeowners branch August 24, 2022 11:30
@oliviertassinari
Copy link
Member

I have created https://www.notion.so/mui-org/Scale-assign-maintainers-88367b09308d4421b4928037874f9e00 so we can use it to collect instances where CODEOWNERS show its limit, and consider how to overcome them.

@oliviertassinari
Copy link
Member

oliviertassinari commented Oct 5, 2022

@mnajdova has identified two problems with the CODEOWNERS process. Because asking a review of each of the maintainers 1. maintainers get a lot more requests for reviews 2. maintainers are no longer able to differentiate a review that was asked automatically from one that was asked specifically. In practice, this means that our attention span is diluted, and it's harder to prioritize our time.

I feel that a better workflow would be to replace it with a GitHub action that once a PR has a "scope" label

a. if a community PR assigns one of the maintainers of the scope to the PR, with a round-robin probability. Then this maintainer is free to ask for a review from another maintainer or the owner or only rely on his judgment.
b. if another peer, ask for a review

daniel-rabe pushed a commit to daniel-rabe/material-ui that referenced this pull request Nov 29, 2022
Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Infrastructure work going on behind the scenes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants