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

Add Team Reviewers to Pull Request #4512

Merged
merged 5 commits into from
Apr 14, 2023
Merged

Add Team Reviewers to Pull Request #4512

merged 5 commits into from
Apr 14, 2023

Conversation

alexr00
Copy link
Member

@alexr00 alexr00 commented Feb 10, 2023

Fixes #1126

@alexr00 alexr00 self-assigned this Feb 10, 2023
@alexr00 alexr00 enabled auto-merge (squash) February 10, 2023 10:32
@alexr00 alexr00 marked this pull request as draft February 10, 2023 10:33
auto-merge was automatically disabled February 10, 2023 10:33

Pull request was converted to draft

@alexr00 alexr00 marked this pull request as ready for review February 15, 2023 14:56
src/github/interface.ts Outdated Show resolved Hide resolved
hediet
hediet previously approved these changes Feb 15, 2023

const cache: { [key: string]: ITeam[] } = {};
const hasAllRepos = (await Promise.all(this._githubRepositories.map(async (repo) => {
const key = `${repo.remote.owner}/${repo.remote.repositoryName}.json`;
Copy link
Contributor

Choose a reason for hiding this comment

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

There could be name collisions with enterprise instances.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we could only get a collision if there are two unrelated enterprises with the same owner + repo name combination. Is that correct?

Copy link
Contributor

Choose a reason for hiding this comment

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

Because the cache key makes no distinction between repo origins, yes, two unrelated enterprises might collide. But more importantly and likely would be a collision between github.com repos and any enterprise.

Example:

  • github.com/microsoft/vscode
  • github.contoso.net/microsoft/vscode

While it may be unlikely that these examples exist and represent distinct teams, they are indeed two different sources of truth for information when querying/caching on the context microsoft/vscode

Copy link
Member Author

Choose a reason for hiding this comment

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

Got it, thanks. I'll update the key in another PR.

@alexr00 alexr00 added this to the March 2023 milestone Feb 16, 2023
@alexr00
Copy link
Member Author

alexr00 commented Mar 22, 2023

Moving this out, I hope for the last time. The reason I haven't merged it yet is that we're waiting to see if GitHub can provide a way to get team reviewers without adding the read:org scope.

@alexr00 alexr00 modified the milestones: March 2023, April 2023 Mar 22, 2023
@alexr00 alexr00 marked this pull request as draft March 30, 2023 09:34
@alexr00 alexr00 marked this pull request as ready for review April 14, 2023 12:56
@alexr00 alexr00 merged commit ec75439 into main Apr 14, 2023
@alexr00 alexr00 deleted the alexr00/issue1126 branch April 14, 2023 13:49
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.

Add Team Reviewers to Pull Request
4 participants