gitflow-avh icon indicating copy to clipboard operation
gitflow-avh copied to clipboard

Support the "pull requests of feature branches with a protected develop branch" workflow

Open dantman opened this issue 6 years ago • 10 comments

I'd like to use the flow model of feature branches and separate stable/development branches, but I heavily use the model where branches are merged through GitHub pull requests and the main branch is protected so no-one can push to it without a PR that can be reviewed and must go through CI first. I've even locked it so even as the repository owner I cannot accidentally push something broken and must follow the same process any other contributor must go through. I believe this is also a good model that fits in with gitflow because it allows you to work on multiple feature branches and have them accepted separately. If you make a pull request from develop instead of from feature/foo then you cannot merge feature/bar into your develop branch until the repository owner has merged your develop branch with feature/foo in it. Otherwise the maintainer will have to simultaneously accept both foo and bar at the same time.

I understand there have been other issues with different suggestions (https://github.com/petervanderdoes/gitflow-avh/pull/208 https://github.com/petervanderdoes/gitflow-avh/issues/327 https://github.com/petervanderdoes/gitflow-avh/pull/330) but I don't quite see why this model can't just be supported by gitflow, it seems to fit in fairly well with gitflow and even enhance gitflow.

Would it be possible to have a flag we can enable for a pull request mode of feature branches.

When the flag is enabled git flow feature finish behaves differently.

If the feature branch has not been pushed to any remote, it behaves as normal (to allow package maintainers to work on local features).

If the feature branch has been published to a remote and has not been merged, instead of merging and deleting the branch the command bails out and warns the user that they should create a pull request for their feature branch. A -f/--force option can be used if the user still needs the normal behaviour.

If the feature branch has been published to a remote and the latest commit in the feature branch has been merged into develop, then the command switches to that commit develop (instead of merging to it) and deletes the feature branch and remote feature branch.

In this way git flow feature finish remains safe and remains a useful portion of the git flow workflow:

  • git flow feature start foo - Start a feature branch as normal
  • git flow feature publish - Publish a remote version of your feature branch to share with the repository owner
  • Start a pull request
  • The package maintainer merges your feature branch into develop
  • git flow feature finish - Checks to make sure that your feature has been merged into develop as it would have done on its own before. If something is wrong it warns you, so you don't accidentally delete your feature branch before it accepted. And if it has been merged and is safe to do so, deletes the feature branch and published remote to "finish" the branch by cleaning it up.

dantman avatar Apr 02 '18 01:04 dantman

I just don't use git flow feature finish... My normal workflow is:

  • git flow feature start ...
  • <make some commits>
  • git flow feature publish
  • <create PR>
  • git pull develop and from time to time I clean local branches no longer needed....

Shea690901 avatar Apr 14 '18 23:04 Shea690901

@Shea690901 Yeah. I do the same.

Problem arises with 'hotfix' branches, when the changes have to be merged in two directions (possibly resolving some conflicts with the develop branch). Yeah, we can always create two PRs, but it'd be nice, if the tool somehow helped. For example - by creating new 'temp' branch for develop backmerge and allowing to create PR from this branch after resolving conflicts etc.

TiS avatar May 15 '18 11:05 TiS

I do what @Shea690901 does as well, but agree that it'd be nice to have direct support for a PR-friendly workflow. Honestly I'm surprised this issue has been sitting open for as long as it has without being addressed directly.

Datamance avatar Jan 03 '19 22:01 Datamance

A "git flow feature pull" would be nice.

thecb4 avatar Jan 30 '19 14:01 thecb4

The need to rebase feature PRs is a nuisance but I think the problem is worst with hotfixes.

The reason is described here: https://discuss.circleci.com/t/github-protected-branch/1121/3

When required status checks are enabled for a branch, pull request made to that branch have an additional requirement – the source branch must be up to date with the target branch (master in your case).

In case of a git flow hotfix, the hotfix branch was based on master branch... if other features have been merged into develop branch in the meantime then when you git flow finish <hotfix> and it merges the hotfix into develop, github will block you from pushing to develop - because the hotfix branch is not 'up to date' with develop branch.

It can be confusing because the error message returned from git will be something like:

remote: error: GH006: Protected branch update failed for refs/heads/develop.
remote: error: Required status check "ci/circleci: build-and-test" is expected.

...but in circleci all the relevant branches may be green. The second error message is spurious, the relevant one is the first one GH006, it fails because this option is ticked in GitHub:

[x] Require branches to be up to date before merging

This setting will not take effect unless at least one status check is enabled (see below).

(I guess this is the reason that "ci/circleci: build-and-test" is expected error comes out as well)

The only way around this that I can find is to follow this procedure:

  1. merge hotfix to master
  2. (can push master, now or later)
  3. merge develop into hotfix (or a new branch from hotfix if you don't want to modify the hotfix itself) and resolve any conflicts if necessary
  4. push the modified hotfix or 'hotfix+develop' branch so that GitHub status checks can run
  5. wait for green status
  6. merge the modified hotfix or 'hotfix+develop' branch to develop
  7. push develop

In my job we are not really using the git flow cli any more (due to using merge button in GitHub PRs), but are still using the git flow branching and merging model. Feature branches are fine (except you need to rebase them before merging to develop) but hotfixes are awkward. Cannot see an easy way around it apart from disabling branch protections at some level.

anentropic avatar Feb 14 '19 13:02 anentropic

Hi everyone. What I do here is:

  1. start feature/release
  2. commit at least once
  3. publish
  4. Open PR
  5. Keep developing, reviewing, etc until it's all green.
  6. finish -k
  7. push target branches. for a feature this is develop, for a release it's both develop and master

The -k here is crucial. It merges and pushes without deleting the branch. If you don't do this your PR will not be marked as merged. NOTE: This relies on the release manager being able to push to develop, as the release PR is to master, but finish merges to both.

tiferrei avatar Oct 22 '19 10:10 tiferrei

@tiferrei That's ok, but, as others said, it requires disabled branch protection at least on some level. In your case - for your user.

I'm quite not fond with that. I'd like, that even I, as repository owner, cannot bypass branch protection during day-to-day work.

TiS avatar Oct 22 '19 10:10 TiS

The lack of this feature is THE reason my team moved away from avh gitflow, if it were to be implemented we would consider moving back

acestronautical avatar Feb 20 '20 18:02 acestronautical

The lack of this feature is THE reason my team moved away from avh gitflow, if it were to be implemented we would consider moving back

Which tool your team are using right now?

guilhermeocosta avatar May 14 '20 23:05 guilhermeocosta

Could git hooks be leveraged to help achieve this? I am very new to gitflow (less than 24 hrs) but can see the real need for this functionality. So in .git/hooks/post-flow-feature-finish can I stop executing the flow action and replace it with mine?

red2678 avatar Jan 16 '21 10:01 red2678