Open hanxiao opened 4 years ago
Hi @hanxiao, First, I am really sorry for responding so late. Thank you very much for taking the time to report this design issue with the GitHub Actions platform. You are absolutely right. CLA bot will be mostly required only for the pull requests coming from the forks.
We are aware of this issue and I myself have raised this issue multiple times in several discussions. In one of the discussion, the GitHub team told that they are working on a fix and they will come up with a solution to address this problem with PR coming from forks. I have already mentioned in the readme file that this action won't work for pull requests coming from forks (see below screenshot)
I really hope that the GitHub team somehow fixes this issue so that we can continue with the development of this action. Please feel free to raise a pull request If you can find a workaround solution to make this action work for PRs coming from forks.
Couldn't you just write at merge time? Granted accepted CLA's that never had their PR merged would have to re-accept, but that's better than this github action being entirely useless for its intended use-case.
Nice idea @Naatan!
Hi @Naatan, Could you please elaborate on your proposal ?.
Do you mean to write the CLA signatures when the pull request is merged?
btw, reducing the frequency of comment posting from the bot is probably something @ibakshay may consider?
recheckcla
to grant the bot write access. Comments are now posted on the thread. Everything works fine.rerun the workflow
on Github action, the bot fails despite everyone has signed the CLA, this is again due to the unnecessary comment posting.Hence, removing unnecessary comment posting especially when CLA pass will improve the usability of this bot.
Example:
https://github.com/jina-ai/jina/pull/470/checks?check_run_id=768552510
Let me clarify the user journey here.
recheckcla
.Hi @Naatan, Could you please elaborate on your proposal ?. Do you mean to write the CLA signatures when the pull request is merged?
Exactly. Obviously not ideal, but with the limitations that exist I think that's the best you can do.
Hi @hanxiao, Thank you very much for the detailed write up and proposal..
now the maintainer could join the thread in and reply recheckcla to grant the bot write access. Comments are now posted on the thread. Everything works fine. next time when the same contributor (signed) makes a PR again, the bot still fails. However, notice that the reason of the failure is purely because of the lack of access of posting "everyone has signed CLA", which I believe is unnecessary (the green check mark in the CICD status will indicate the success of this action anyway).
Awesome insights 💯. I never thought about this. You are absolutely right. Unnecessary comment posting from bot is not required at all if all the committers have already signed the CLA. I will roll out a release soon to remove the unnecessary comment from this bot. Also, Feel free to raise a pull request for your proposal :).
Exactly. Obviously not ideal, but with the limitations that exist I think that's the best you can do.
I believe pull_request.closed
also belongs to pull_request
event. Unfortunately, there is no write access for any type of pull_request events coming from the forks. For getting more insights, you can read this thread. I hope GitHub finds a solution soon..
The plan sounds really good! I'd note that the first time the check fails, it could print what the user should do in the logs. Currently it only says it failed to post a comment (it should probably check if it has access first and fail with a better error).
Hello @ghuntley @hanxiao @domenkozar @Naatan :)
Today, GitHub Actions rolled out a series of updates, and Pull Requests from forks are supported 😄 . We have to make use of this new event pull_request_target
instead of pull_request
event. So, this action will be fully usable from now on.. I will roll out a new release this week which will reflect these changes :).
Hello again @ghuntley @hanxiao @domenkozar @Naatan :)
Today I drafted a new release v2.0.0-alpha and in this new release, Pull request coming from forked repository is supported with many other features, improvements and bug fixes :) .
Thank you! I will upgrade today/tomorrow.
@ibakshay how come PAT is now required?
We, unfortunately, need a PAT to access this API to re-run the previously failed CLA PR status checks when the contributor has signed the CLA. I have also asked about this thing, in the GitHub community forum
Before, we need to manually re-run the previously failed CLA PR status check. So, PAT was not required.
Wouldn't it be possible for the action now to push a commit to PR that would trigger the build?
EDIT: nevermind, action wouldn't run.
Hi @domenkozar, I am sorry for the late response. I was on vacation 🌴.
Yes, you are right. Push event from the GitHub Acton wouldn't trigger the action workflows.
I even created a ticket in the GitHub Community discussion regarding this issue and come to know that, it is not possible :/ .
You can view my ticket in the GitHub Community forum here
First, Great work!
Please be careful when you continue developing this action. For a pull request coming from a fork (which is where many opensource projects' contribution coming from and thats why people need a CLA bot), Github Action grants only a read access on GITHUB_TOKEN; and PAT secrets are not available on forks. These two restrictions will greatly limit the usage of this action. We already encountered this problem: https://github.com/jina-ai/jina/pull/417
Please refer to this issue for the same problem: https://github.com/actions/labeler/issues/12 A lot of users adapt to this Github action and later have to remove it from their CICD pipeline. You can scroll it down and see a lot of PRs related to that issue are just for removing it from the pipeline.
A good way to test it is let some user fork your repo and submit a pull request from their fork.