-
Notifications
You must be signed in to change notification settings - Fork 107
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
Automate code review assignment #613
Comments
I like the idea, sounds like a great improvement. Thanks for researching that! |
I have to add some cents to that topic. With wide variety of Plug-Ins Prow can:
In addition Prow is also expendable with API for handling the external plugins which might further extend the workflow related operations. For more information regarding the Prow flow refer to the following docs: The example PRs handled by Prow can be seen in any repo in Kubernetes organisation. |
Maybe both suggestions can be combined? So, use area teams to support automatic assignment, and use the Prow functionalities to manage the entire workflow. Not sure if that works from a technical perspective though. |
Unfortunately GitHub assumes all codeowners also have write access to the repository, as stated in official GitHub docs.
If we were to revoke the write access from members then GitHub's functionality won't work. Additionally Prow does not allow to define GitHub groups as reveiwer/approver in the OWNERS file because GitHub does not provide audit logs for changes in the teams. Hence why there is OWNERS_ALIASES, where you can simply define aliases for groups of people. It's convenient and simple. |
@Ressetkk I am not attached to the GitHub solution if we can achieve the same result with OWNERS and ONWERS_ALIASES we can try it. If only |
@pbochynski I have prepared the workaround regarding this problem - Prow plug-in that will keep blocking label until a person from tectonical writers approve changes, and in the future Prow will just allow for granual approvals. However teams responsible for the code will have to update or create the OWNERS file inside their directories before we enable Prow approval flow. |
We agreed with @Ressetkk to apply prow plugins in some repositories and verify the code review flows there (e.g. test-infra or community repo). |
After a week of extensive testing I'm here to write some conclusions:
Of course it's an intended thing, as teams may have different review flow for PRs that touch their Prow also helps defining which person to
|
Two points from my side
|
Not that I'm aware of, but once the OWNERS files will be filled up and have
Prow will most likely ignore the second option, as the new flow will not be using GitHub rules. AFAIK |
I have made some tests with issue/PR assignments to Projects and Milestones. We've created test GitHub Team called Projects: Milestones: It's also possible to automatically assign PR to the milestone. Prow also provides such functionality - based on the plugin configuration, The milestoneapplier plugin automatically applies the configured milestone for the base branch after a PR is merged. If a PR targets a non-default branch, it also adds the milestone when the PR is opened. That means if the PR is merged to the The tests were done on Issue kyma-project/test-infra#4832. |
Would it be possible to add new issues to new projects automatically without Prow plugin but with some external script that are calling New GitHub projects API directly with some technical account that has the write permissions? |
If I understand correctly - the only way to add milestones/projects to the issue would be to go through the board for projects and with the use of commands in GH comments for milestones? Looking at the test issue - it kills the transparency for me. We want to have status updates in the issues, those will be mixed with commands for milestone updates (and we have lots of those). Regarding projects: GH-default fields (such as assignee or labels) are immutable in the project as well. Isn't that a little bit - overkill? |
@janmedrek status update comments can include commands, as well as Issue/PR body. That is really a change of a habit IMO. Problem is when person has excessive permissions for the repository and unfortunately GitHub doesn't want to improve on that so easily. For example one of the most requested issue was to restrict pushing tags to the repository. No progress since 2019. With bigger orgs such things may generate more problems than solved issues. |
Hi @Ressetkk ! 👋 Thank you for your presentation on SIG Quality! 🙂 I just want to confirm that in the new approach nothing will really change from the TWs perspective, is that right? We will operate on this In case you'd like to change that in the future, I have two remarks:
cc: @kyma-project/technical-writers |
Prow will only add label to the PRs that have changes in the Markdown files. It will not request any reviews from TWs and will not require reviews from 2 people from this group. Only one approving review from Of course there is still room for improvements and if you have any suggestions please raise an issue on the
The workaround that is working on the test-infra will not change the flow you review the documentation. Manual ping still will be needed for documentation review. Either in GitHub or by other means. |
@Ressetkk what concerns me is that we can always have ppl in github team who shouldn't be codeowners. i assume that everyone in a github team is already codeowner, right? |
@strekm code owners are defined in the |
Totally agree with that, I am not convinced of the commands though. We are automating one action (assigning a reviewer) while forcing multiple other actions (such as adding labels or milestones) to be done via commands - this is inconvenient in comparison to just choosing a label from the list, it will generate a lot of noise since reviewer needs to be assigned once and issue metadata is something that changes quite often (looking at the stalebot for example). |
I generally like the idea of specifying teams and groups of people as codeowners of some functionality in Kyma or control-plane. But the idea of automatic review assignment for PRs is problematic to me and I believe it will negatively impact the quality of the code. Here are my thoughts:
To summarise my opinion I strongly believe that the team should agree on the matter who should review individual PR - not random algorithm. |
That's why I suggested that there should be a specific set of people (POs) that have Triage GitHub role. Proper GitHub Team can be created, and not only have the Triage role, it can be configured to be used with Choosing a label using specific set of commands restricts your usage of some protected labels. Tide uses labels to determine the status if PR can be merged or not. Right now you can modify labels to your liking by using any of the labels, also the ones you are not supposed to use, like |
With Granular OWNERS you can define for example a SUBSET of people from framefrog that should review or approve the PR if it touches the file. Let's take an example: you have created a package for component your team is responsible for. This was the combined effort of 3 people in your group. You can define OWNERS file in this directory with OWNERS file can include specific GitHub usernames or aliases from OWNERS_ALIASES file. This will not change your usual way of reviewing the PRs. If the OWNERS file are properly configured, then correct people will be assigned to do a review. If not, one can always |
My short comment about this. The problem I see with automatic assignment is that it can assign people who doesn't contribute to kyma any longer. |
Based on the
With Prow integration you will be able to define multiple OWNERS files for each directory, see http://github.com/kyma-project/test-infra for reference. |
good point. I think we can extend this proposition to create new labels (for example Here is an example link to filter all PRs in the |
@pPrecel Maybe instead of creating labels for With properly created OWNERS files automatic review assignment should be pretty straightforward. I have also suggested similar approach in my previous messages. Please take a look at them.
|
@Ressetkk Asigning PR review for the team with just a GH label could be the easiest approach without defining specific owner for every file. With labels we can be quite flexible and dispatch review request to developers who have time to take care about this task. IMHO that could make our life much easier. |
If specific teams want to adjust their review flow differently I see no problem if said teams just ignore automatic assignment and dispatch review in their own flow. As I mentioned before, everyone from the organisation can do a review, and add We want to provide secure repository management and reliable infrastructure with good set of automations that improve workflow in the organisation. |
We're using I just wanted to point out that some fields that are easily edited via GitHub Projects boards won't be editable anymore in such convenient way when we revoke the write accesses. |
One more scenario to consider. We often tend to create issues, which contents are later edited by someone else by adding e.g. some details. Will that be possible without having write accesses on the repository? I understand that we can add new comments, but sometimes it makes sense to just edit the original description of your team mate so it will be easier to see all essential information in one place. |
@Disper no, only people with Write access can modify others messages. It's actually a security measurement since anyone can modify anyone's message completely changing the original message. |
@Ressetkk |
@dbadura Yes. |
This issue has been automatically marked as stale due to the lack of recent activity. It will soon be closed if no further activity occurs. Thank you for your contributions. |
Because prow approval flow was reverted neighbors team decided to use github native auto code review assignment. This feature can is enabled on a team level. This doesn't require change on a repo or organisation level. With our monorepo setup it allow each team to decide if they would like to use it. Github documentation with instruction about feature enabling. https://docs.github.com/en/organizations/organizing-members-into-teams/managing-code-review-settings-for-your-team#configuring-auto-assignment. Because of enabled standard branch protection on our repositories a team review request is not removed from the list. However auto assignment notification settings allow send notification to the individual members only. This makes web or mail notification less noise.
If you are using github scheduled reminders you can make it ignore team review request assignments too. One requirement which some might see as a drawback is that you need to use a GitHub team as an owner in CODEOWNERS file. |
Thanks a lot for the provided details and the suggestion to use Github auto-assignments for reviews. We enabled the feature for our existing reconciler repository and are looking forward to gain more experiences about its impact on our current "merge performance". It looks quite promising that we can shorten the time between opening and merging a PR. |
This issue has been automatically marked as stale due to the lack of recent activity. It will soon be closed if no further activity occurs. Thank you for your contributions. |
This issue has been automatically closed due to the lack of recent activity. /lifecycle rotten |
As a tendency is now to move from big monorepo into teams repositories problem can be addressed on team level using github features. |
Description
Code review assignment should be automated to:
It can be done by enabling auto-assignment in the team settings.
To make it effective we need first to refactor our CODEOWNER files to use teams. The idea is to not use internal team names (can be confusing for externals or newjoiners) but area teams. Developer can join multiple teams so we can be still flexible. We can simplify CODEOWNERS file and keep it stable (no need to change CODEOWNERS when someone join/leaves the team).
Plan
Reasons
If your Pull Request (PR) involves many code owners it is hard to get them working on it. You need to actively search for the proper team member on slack as the notifications are going to all code owners and none of them feel responsible for the review (they get tons of such notifications). As a result, the review process generates also notification hell on slack. The process is inefficient and almost impossible to go through for external contributors (they do not know who to ping on slack).
The text was updated successfully, but these errors were encountered: