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

Prevent Non Collaborator Aliases Version 2: Using Permission API and Batch Call #6

Merged
merged 10 commits into from
Apr 19, 2024

Conversation

jamoor-moj
Copy link

@jamoor-moj jamoor-moj commented Apr 15, 2024

In a previous PR (#5), the request reviewer call was converted from a batch call to multiple individual calls in order to support cases where an alias no longer has access to the repo.

In very rare circumstances, it has been reported that the action will actually remove add (and then automatically get removed) from the pull request. Since the action doesn't invoke any remove reviewer APIs, the only theory so far is that making multiple edit request simultaneously has exposed a rare syncronous problem, usually when combined with auto assignment. This scenario is rare enough that it has been difficult to get reproductions of the issue in test environments.

The proposed solution is to instead async request the permission status of every alias the action will attempt to add. After locally filtering out all aliases that do not have permissions, make a batch request with all of the remaining aliases. This adds 1 extra network call compared to the solution in 5, but reduces the number of network calls attempting to edit the PR down to 1.

Copy link

@AmyBernhoft AmyBernhoft left a comment

Choose a reason for hiding this comment

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

It's been such a long time that I've had to look at javascript. Still approving even though my javascript is rusty.

@jamoor-moj jamoor-moj merged commit 4515b0a into master Apr 19, 2024
1 check 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