-
Notifications
You must be signed in to change notification settings - Fork 283
Assign pr reviewer stratconn #3210
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
Conversation
.github/workflows/configure-pr.yml
Outdated
const script = require('./scripts/github-action/assign-reviewer.js') | ||
await script({github, context, core}) | ||
- name: Log Skip Reviewer Assign Reaso |
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.
- name: Log Skip Reviewer Assign Reaso | |
- name: Log Skip Reviewer Assign Reason |
if (teamToAssign !== 'strategic-connections-team') { | ||
core.setOutput('skip', 'true') | ||
core.setOutput('reason', `only assigning for strategic-connections-team, found ${teamToAssign || 'none'}`) | ||
return | ||
} |
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.
What happens if the team is actions:mappingkit
? How do they get notified of the change? Will they be monitoring the repo themselves?
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.
rewrote the logic based on codeowners file.
return | ||
} | ||
|
||
// Get a random team member |
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.
Hi @monutwilio
Apart from mappingkit (and possibly some other components) the All PRs raised by people outside of our immediate team should be reviewed by me.
So there's no need to assign those PRs randomly.
My role is to work with 3rd party Developers to build Integrations, so I need those PRs to be assigned to me. Currently I do this assignment manually.
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.
This looks great! ✨ . Left couple of questions.
if (allLabels.includes('actions:mappingkit')) { | ||
return 'libraries-web-team' | ||
} |
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.
This should be our team or https://github.com/orgs/segmentio/teams/data-activation-namer team. Not libraries-web-team
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 think this might conflict with .CODEOWNERS. should we instead look at codeowners for the team and then pick one or two from that team. we can have a special rule for @joe-ayoub-segment
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.
Made the change, now the flow is.
- When the PR status is not draft (either open as ready for review, or changed the status from draft), github assigns the team as reviewer based on .codeowners file.
- We pick the assigned team to assign the reviewers.
|
||
if (teams.length > 0) { | ||
// Return the first team | ||
const selectedTeam = teams[0] |
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.
Sill this always select libraries-web-team
for browser-destinations
? Is this deliberate?
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.
We can edit codeowners file if we want to assign browser-destination PRs to some other team.
Looks good to me. Is it possible to put a summary at the top as a comment to explain the entire flow? @varadarajan-tw is that OK or bad practice? |
This PR adds a step in label-pr workflow to assign a reviewer for PRs assigned to stratconn team and renames the workflow to configure-pr.
Summary
This PR introduce changes to assign reviewers to PR based on below logic.
Testing
Include any additional information about the testing you have completed to
ensure your changes behave as expected. For a speedy review, please check
any of the tasks you completed below during your testing.