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

bot: Allow the same reviewer in both core and cloud repos #207

Merged
merged 5 commits into from
Jan 3, 2024

Conversation

jimbishopp
Copy link
Contributor

This PR addresses the "Cross-Repo Reviewers" issue described in #205.

The current bot configuration data model doesn't allow an engineer to be a reviewer in both the cloud and core repositories. This is an issue because we now require the core team to submit PRs to the cloud repo to manage either tools they own (e.g. release server) or deploy a new version of a component they own (e.g. TAG).

This PR provides a hacky solution that will allow us to change the bot config data model to include a cloud and core team section, while keeping the original code reviewers map in the bot so we don't have to change a large amount of code.

@jimbishopp jimbishopp changed the title Allow the same reviewer in both core and cloud repos bot: Allow the same reviewer in both core and cloud repos Dec 26, 2023
@jimbishopp jimbishopp changed the base branch from jim/approver to main December 26, 2023 23:58
@jimbishopp jimbishopp changed the base branch from main to jim/approver December 26, 2023 23:58
@jimbishopp jimbishopp force-pushed the jim/cross branch 3 times, most recently from 1a4005d to d85387b Compare December 28, 2023 17:07
Copy link
Contributor

@zmb3 zmb3 left a comment

Choose a reason for hiding this comment

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

Can you include an example of what the new reviewers config will look like? I'm having a hard time telling exactly how this changes things.

bot/internal/review/review.go Outdated Show resolved Hide resolved
@@ -132,6 +132,14 @@ func (e *Environment) IsCloudDeployBranch() bool {
(e.UnsafeBase == CloudProdBranch || e.UnsafeBase == CloudStagingBranch)
}

// Team returns CloudTeam when the repository is the cloud repo otherwise CoreTeam.
func (e *Environment) Team() string {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
func (e *Environment) Team() string {
func (e *Environment) RepoOwner() string {

Perhaps?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to RepoOwnerTeam since it returns a Team name.

@jimbishopp
Copy link
Contributor Author

jimbishopp commented Dec 29, 2023

Can you include an example of what the new reviewers config will look like? I'm having a hard time telling exactly how this changes things.

@zmb3 the test was updated to demonstrate what the new config will look like. Essentially codeReviewers will be replaced by coreReviewers and cloudReviewers. I also removed the Team attribute and set it when loading the file. I marked it as deprecated for now because it is baked into so much logic but it can be removed if we refactor the code to determine the team based on the environment/repo. I think eventually we'll need to have a data model that defines reviewers per repo but I didn't want to take that on right now.

UPDATE: opened a PR in the github-terraform repo with changes to the reviewers.json file.

const reviewers = `
{
"coreReviewers": {
"1": {
"team": "Core",
"owner": true
},
"2": {
"team": "Core",
"owner": false
}
},
"cloudReviewers": {
"1": {
"team": "Cloud",
"owner": true
},
"2": {
"team": "Cloud",
"owner": false
}
},

@jimbishopp
Copy link
Contributor Author

I added commit that removes the need to set the Team field in the reviewers.json configuration file. It is now set automatically when the file is loaded.

@jimbishopp jimbishopp changed the base branch from jim/approver to main December 29, 2023 19:51
@jimbishopp jimbishopp marked this pull request as ready for review December 29, 2023 19:51
@jimbishopp jimbishopp requested review from a team and zmb3 December 29, 2023 19:51
bot/internal/bot/bot.go Outdated Show resolved Hide resolved
@jimbishopp jimbishopp merged commit 5cb9e73 into main Jan 3, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants