KEMBAR78
Add Team Reviewers to Pull Request by alexr00 · Pull Request #4512 · microsoft/vscode-pull-request-github · GitHub
Skip to content

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
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

5 participants