ansible-community / collection_bot

Fork of Ansibullbot for collections
GNU General Public License v3.0
5 stars 10 forks source link

Allow to have certain branches inside the repository #9

Closed felixfontein closed 3 years ago

felixfontein commented 3 years ago

This is needed for bot backporting (to avoid unnecessary hassle with having a forked repo the backport bot needs to manage).

We tried @webknjaz's patchback bot today on community.general, and it worked, except that ansibullbot closed the backport PR: ansible-collections/community.general#882 with @patchback[bot] Please create a new PR using a branch in your fork..

(I guess the reason for this behavior is to avoid double CI runs in Shippable, but these can be prevented for special branch names, see for example https://github.com/ansible-collections/community.general/commit/2b0879cdc4db1fba684acfe1f6d447285b8e979b.)

gundalow commented 3 years ago

No branches in main repo also avoids everybody having to checkout these branches.

@webknjaz what do you think of the bot creating branches in it's own fork?

gundalow commented 3 years ago

Ah, didn't read all of @felixfontein comments. We could add the backport box to an allow list.

gundalow commented 3 years ago

1) Config option to define allow list 2) update conditional at https://github.com/ansible-community/collection_bot/blob/b15aabf4458fb90f50c500307cd3f959f2b60242/ansibullbot/triagers/ansible.py#L854:L861

webknjaz commented 3 years ago

@gundalow making it work with forks would make the bot unnecessarily complicated. Patchback is supposed to be generic and so I'll be trying hard to avoid hardcoding things. Back in the day, I was considering making it for ansible/ansible among other things. To achieve this, it'll need a per-repo config under .github/.

Just to give a taste of what it'd take to make it work with forks. The simplest case is having a special org with repo forks. That org would have Patchback installed but would need to be special-cased. Patchback would need to maintain the fork mappings and would need to push to repos in that org. This means that it'll likely hit rate limits sooner or later because they are per install (hence per-org). To mitigate this, we'd need to make it possible for every repo to specify its own fork location making it the end-user's responsibility to set up some org (or their own user account) with forks and make sure to install Patchback there too (so that it'd have access to the fork repos). Not sure how helpful this would be, though. Plus, it'd probably not work with private repos. As you can see, it's very easy to increase the complexity disproportionally to the usefulness of such an effort. AFAIR pyup used to do something like this but they've migrated to having in-repo branches eventually. I guess dependabot also does this for similar reasons.

There's one thing, though, you can (and probably should) do to decrease the number of the non-main branches — there's a checkbox in repo settings that tells GitHub to remove PR branches upon merge. The contributors will still encounter some branches but hopefully not as many. I guess I could make bot remove the branches right after creating the PRs but this would mean that maintainers will only be able to accept PRs as is vs being able to tweak things in those branches before merging so it's a highly controversial idea.

re: updating collection_bot — GitHub submitter object has an account type field that is either User or Bot. Using it, it's easy enough to make a generic check for is_bot (that will work for any GitHub App but not for bots based on human-like accounts). I'd suggest doing this and only add an allow list if there's extra bots that are humans, in the future. I think this'd need adding an extra method next to this place https://github.com/ansible-community/collection_bot/blob/90c47b205087d212bc35cdc8860c3a0f939631c8/ansibullbot/wrappers/defaultwrapper.py#L522-L525 to retrieve the user type.

P.S. Please post any ideas for patchback @ https://github.com/sanitizers/patchback-github-app/issues instead of this place so that they could be incorporated in generic fashion.

gundalow commented 3 years ago

@webknjaz Thank you for the suggestion about deleting merged branches. I've enabled this on: