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
[core] Specify code owners #33995
Conversation
2115e59
to
96d7d90
Compare
There was a problem hiding this 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:
- https://github.com/marketplace/actions/reviewer-lottery
- https://github.com/marketplace/actions/review-assign-action
- https://github.com/marketplace/actions/required-review
- https://github.com/marketplace/actions/team-request-review
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)
}
})();
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.
- The Notion app: https://www.notion.so/my-integrations/internal/ed30e642e40a41f687f3c6f0a4b658d7
- The access needs to be granted per page, e.g. https://www.notion.so/mui-org/0c8156b0b1634ea5bfd903dd005ec1cf?v=b9e94cc3ea554e668e7c54d585910569
/packages/mui-system/ @mnajdova @siriwatknp | ||
|
||
# Components - Inputs | ||
/packages/mui-material/src/Autocomplete/ @michaldudak @mnajdova |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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>
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. |
There was a problem hiding this 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
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. |
@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. |
Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
Specified owners of different areas of the code according to https://www.notion.so/mui-org/Product-owners-836c72feffcc4cb2b735b51527e6991a
About code owners: https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/customizing-your-repository/about-code-owners