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

Allow Linux CI to push changes to forks #2182

Merged
merged 59 commits into from
Feb 16, 2022
Merged

Allow Linux CI to push changes to forks #2182

merged 59 commits into from
Feb 16, 2022

Conversation

ZanSara
Copy link
Contributor

@ZanSara ZanSara commented Feb 14, 2022

Right now, the documentation and code style bot will fail on forks as the checkout@v2 action fails. This leads to the situation in which no code style or documentation will be updated on the PR and neither on merge to master (the bot can't push to master right now), but they will trigger on the first PR opened from the updated master.

This PR tries to make checkout@v2 push changes back to the source fork, if possible.

@ZanSara ZanSara added topic:tests type:bug Something isn't working labels Feb 14, 2022
@ZanSara ZanSara marked this pull request as ready for review February 15, 2022 09:11
@ZanSara
Copy link
Contributor Author

ZanSara commented Feb 15, 2022

Summary of the solution I've found:

  1. It seems to be not possible by design, for a bot that runs on the original repo, to push changes back to a contributor's fork.

  2. CI workflow triggers, however, can trigger on either the original repo and the forked one: specifically, push runs on the fork (which sees the push event) and pull_request works on the original (which receives the PR request).

  3. By using push as a trigger, the action always run with a token that has permission to commit back (it stays local, in a way).

  4. I made "Linux CI" work on pull_request and "Code & Documentation Updates" work on push

  5. Gotchas:

    • Contributors have full control on the actions that run on their side and can disable them completely
    • We have limited visibility of what's happening on their end
    • We will not see the status of their actions on our side, we have to trust they run them
    • On the other hand, if they don't run the actions, the only issue would be to have some "spurious" Black and docs changes on other, unrelated PRs, which might slightly conflict one with the other. Annoying but not critical.

We could implement a "Verify Code & Documentation Updates" action on our end which does not try to commit back the changes, but will always fail if it triggers: that's good because on our end such action would trigger only if the the contributor has not run the corresponding action on their end.

@julian-risch I am in the process of implementing the last "verification" action, feel free to start reviewing before and let me know if you see something that needs change.

Copy link
Member

@julian-risch julian-risch left a comment

Choose a reason for hiding this comment

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

Looks good to me so far. Just add a brief description to the contributor guidelines: https://github.com/deepset-ai/haystack/blob/master/CONTRIBUTING.md please. Maybe even with a screenshot of what the contributor needs to do to run the GitHub action?
Let's test the process ourselves with an account that is not in the deepset GitHub organization yet (once your changes are merged).

@ZanSara ZanSara merged commit 4e940be into deepset-ai:master Feb 16, 2022
@ZanSara ZanSara deleted the ci_push_on_forks branch February 16, 2022 15:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic:tests type:bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants