Makes sense, I didn't think about automatic synchronization. I agree that we should not force push to master (we have discussed renaming to "main"), so using revert commits when there is a mistake sounds fine, too.
On Sat, Jul 24, 2021 at 12:09 PM Jarek Potiuk <ja...@potiuk.com> wrote: > > Also one comment for force-pushes to the main branch - I did it twice > before (before we enabled protection) and It turned out that it broke some > of our users whole setup automated synchronization of 'airflow' repo (they > added automated patching of they patches on top of our repo and > force-pushed changed broke their script. So we decided to enable branch > protection and NEVER force-push the mai branch. > > On Sat, Jul 24, 2021 at 7:05 PM Jarek Potiuk <ja...@potiuk.com> wrote: > > > Hey Wes, others. > > > > It also protects you if you merge/push with git commands. > > > > I stopped worrying and double-checking if I have the "right" remote when I > > push directly to master when we enabled the protection because I simply > > can't push directly to master of "apache/airflow" any more (unless I am > > pushing the tip of a branch from PR that is already approved). > > > > GitHub has a surprisingly good level of integration of regular git > > commands with Git UI ones and the protection works in both cases. As long > > as you use PRs from GitHub, this protection also protects you from any git > > commands that directly manipulate your protected branch (unless the > > "manipulation" comes from an approved PR). > > In the past we had similar cases, and we also had a few different patterns > > of merges (some with Git commands, some with UI ones). For example not > > everyone knows that if you have a PR opened in your repository (not from > > fork) and go with squash&merge, as long as this PR is approved you can > > still fast-forward your main branch to the tip of the branch from the PR > > and avoid squashing (but if your main branch is protected, such > > fast-forward will fail with the message that you need to have approval). > > > > Actually I think GitHub went above and beyond with this feature - the > > rules they have are pretty sophisticated and take into account many > > scenarios: > > https://docs.github.com/en/github/administering-a-repository/defining-the-mergeability-of-pull-requests/about-protected-branches#about-branch-protection-rules > > > > J. > > > > On Sat, Jul 24, 2021 at 6:32 PM Wes McKinney <wesmck...@gmail.com> wrote: > > > >> hi Jarek, > >> > >> We don't merge with the GitHub UI, so I am not sure if that will help us. > >> > >> Weston, in the future if you make a mistake like this and notice right > >> away, IMHO it's acceptable to force-push master reverting the mistake. > >> If some time has lapsed then reverting is better. > >> > >> I have had my git checkouts all set up so that pushing to apache/arrow > >> can't happen unless I explicitly use the apache branch target > >> > >> git push apache $BRANCH > >> > >> Everyone should ensure that the "origin" branch points to your fork > >> rather than apache/arrow. > >> > >> - Wes > >> > >> On Sat, Jul 24, 2021 at 2:32 AM Jarek Potiuk <ja...@potiuk.com> wrote: > >> > > >> > FYI. You can protect branches with .asf.yaml against those kind of > >> > incidents: > >> > > >> https://github.com/apache/airflow/blob/8e94c1c64902b97be146cdcfe8b721fced0a283b/.asf.yaml#L43 > >> > > >> > On Sat, Jul 24, 2021 at 7:04 AM Weston Pace <weston.p...@gmail.com> > >> wrote: > >> > > >> > > Thanks. I went ahead and did that. > >> > > > >> > > On Fri, Jul 23, 2021 at 6:53 PM Mauricio Vargas > >> > > <mavarga...@uc.cl.invalid> wrote: > >> > > > > >> > > > I think that a 2nd commit + push with the reverse shall be the best > >> fix > >> > > > > >> > > > On Sat, Jul 24, 2021 at 12:41 AM Weston Pace <weston.p...@gmail.com > >> > > >> > > wrote: > >> > > > > >> > > > > Well it did not take long for me to make a mistake. I > >> accidentally > >> > > > > pushed a commit to master instead of my remote when creating a PR. > >> > > > > What is the best way to remedy this? Push a revert? Since it is > >> > > > > protected I cannot force push to reset back to where it was (not > >> that > >> > > > > I would necessarily want to do that). > >> > > > > > >> > > > > I have updated my master and removed the upstream (that probably > >> > > > > should never have been there) so hopefully this won't happen > >> again. > >> > > > > > >> > > > > -Weston > >> > > > > > >> > > > > >> > > > > >> > > > -- > >> > > > — > >> > > > *Mauricio 'Pachá' Vargas Sepúlveda* > >> > > > Site: pacha.dev > >> > > > Blog: pacha.dev/blog > >> > > > >> > > >> > > >> > -- > >> > +48 660 796 129 > >> > > > > > > -- > > +48 660 796 129 > > > > > -- > +48 660 796 129
