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

Use the config from current branch instead of default branch #34

Merged
merged 3 commits into from
Feb 23, 2021
Merged

Use the config from current branch instead of default branch #34

merged 3 commits into from
Feb 23, 2021

Conversation

hex0cter
Copy link
Contributor

@hex0cter hex0cter commented Feb 22, 2021

The current implementation uses the configuration file from master branch instead of the active branch. This is not really aligned with how github actions config works in general.

If a user wants to use this plugin for the first time, he needs

  1. create a pull request and add the configuration file (.github/auto_request_review.yml) into master without testing it.
  2. create a second pull request to add the workflow and test the config file from step 1.

If we use the current branch instead, the two steps above can be merged into a single step: both the config file and workflow can be added by one Pull Request.

P.S. @necojackarc the error in the build checks seem to be related to permission. Unfortunately there is nothing I can do about it. Please let me know if I should bump the version in package.json within this pull request. Many thanks for making this action!

@necojackarc
Copy link
Owner

@hex0cter Thanks for you pull request!

If we use the current branch instead, the two steps above can be merged into a single step

Yeah, I think that's a fair point. The reason why that's implemented in that way is that I'm a bit worried about the situation where you can be removed from reviewers without you knowing it if you use the current branch. If you wipe out all reviewers in a pull request, no one will be assigned as reviewers.

But I feel like I'm trying to address some unlikely event at the sacrifice of convenience and some sort of naturalness.
Do you have any thoughts on this point? I'll sleep on it tonight and make a decision but any thoughts are welcome!

the error in the build checks seem to be related to permission

Yeah, I'm not yet very familiar with what secrets.GITHUB_TOKEN is but it looks like the default token of the author of PR, so it doesn't have necessary write access to this repository. I think I should make a token with the bare minimum permissions to assign reviewers.

@necojackarc
Copy link
Owner

I added a ticket for the token permission issue - #35

@hex0cter
Copy link
Contributor Author

@necojackarc Thanks for sharing the thoughts. You have a fair point as well. 😄

I think the only drawback is whenever someone updates the configuration file (.github/auto_request_review.yml), there is no way to test it before merging it.

If you decide to keep the current implementation, it would be good to mention the idea behind it in the documentation so people don't have to debug it in the future. 🥰

@necojackarc
Copy link
Owner

@hex0cter Thanks for your feedback. I'll be merging this PR as-is since such an event that I'm worried about is quite rare and probably, I should trust people a bit more 😄

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