Proposal for Prow mechanism for force-pushing a feature branch

[mdlinville]

Yesterday, @chenopis and I met with @fejta to discuss the workflows used by sig-docs, and started to discuss how Prow might be used to accomplish the rebase/force-push workflow to keep the base of a feature branch up to date, while obeying the CLA requirements and other testing requirements. Here is a potential workflow that could be enabled by Prow. I'm putting this here for discussion. @fejta said that other teams also really need this workflow.

1. The person maintaining the feature branch rebases it on its parent (perhaps master) locally and pushes the rebased branch to their fork.

2. The person then creates a pull request comparing the rebased branch, not to the current state of the feature branch, but on the thing it was based on. For instance, a release-1.11 branch that was based on master might be named rebase-release-1.11 on the person's fork, and the PR is raised between rebase-release-1.11 and master. This exposes the commits that differ from its base and allows people to review the state of the commits and the resulting branch after the rebase / conflict resolution.

3. An approver issues a prow command like /force-push , which directs Prow to force-push the feature branch to reflect the state of the branch in the PR. Probably there should be some protection against Prow being able to force-push into master itself, or maybe a list of forbidden branches.

4. When the PR is approved (maybe an extra approval bit is needed beyond the lgtm and approve ones now, maybe force-push-approve), Prow temporarily unprotects the branch, does the force-push, adds an extra commit to document that the force-push happened and what PR it was requested from, then reprotects the branch.

5. The PR itself is closed since it was only a vehicle for review and for controlling Prow.

cc/ @kubernetes/sig-docs-maintainers for visibility.

[chenopis]

There needs to be a way to differentiate a branch as a feature branch. Perhaps the branch whitelist for each repo in the prow config file would designate the locked down branches that cannot be rebased, such as master, and everything else is treated as a feature branch that would support this workflow.

[fejta]

/cc @lavalamp for his feature Branch thoughts

[lavalamp]

We decided not to do this for our feature branch. What is wrong with just merging like git was intended?

[lavalamp]

(If you did want to do this I'd suggest making the directive be in the body of the PR, not an imperative command someone gives at the end. I.e., it's about the type of the PR: it's /type replace or /type merge (default).)

[mdlinville]

@lavalamp the Docs team has non-release-specific improvements always going into the master branch, so that we do not need to wait for a release to happen to get improvements like typos and things like the Hugo migration live. The alternative is to publish from a release branch, which will only get typos and other non-release-specific improvements when a new release goes out. This doesn't best serve users of our docs, as something may be fixed for months before a reader gets any benefit from it.

In a similar way, we need to have feature-specific work for 1.11 in the "feature branch" release-1.11, which is based off master. When we rebased release-1.11 on master after the Hugo migration, we got Hugo in release-1.11 and the release-specific PRs remained at the tip. Also, we do not need to manually cherry-pick each typo and structural commit to master, back into release-1.11 -- we just periodically rebase and force-push it. It is a lot tidier to maintain and doesn't break PRs that are in-flight when you need to incorporate the new base. The merge workflow would require each PR submitter to periodically merge release-1.11 into their PRs and would be a big mess. (I have done it both ways, many times in my career).

This is similar to other feature-branch work that is built on top of a well-known base. In the feature-branch workflow, all feature commits should go on top of the base, and the base needs to be periodically updated. Nobody cares about the chronology of commits in the feature branch until the feature is merged, at which time all feature-related commits will be at the tip.

Merging into feature branches muddies the water around chronology and makes it very difficult to refactor commits in the feature branch (as should be pretty easy to do, since the feature branch is not yet definitive).

I am happy to jump on a call with you about this, as we did with @fejta earlier this week.

The problem with putting the imperative in the PR the way you propose is that you make the PR between the rebased branch and what you rebased it against, rather than the actual feature branch. If I made a branch called fix-1.11 based off release-1.11 and then rebased it on master, I have to make my PR between fix-1.11 and master, not against fix-1.11 and release-1.11, in order for Github to be able to make sense of what it's seeing. But then Prow doesn't have any indication of what the target for the force-push should be, unless you include it in the syntax. So maybe /type replace and /targetbranch release-1.11 together would be sufficient.

[mdlinville]

Also I am having a hard time understanding "like git was intended" because we are not proposing using commands that are not part of Git. Can you clarify?

[stevekuznetsov]

I think the question was -- why not operate like this:

master: A --- B --- C --- D --- E --- F
feature:      `-- 1 -- 2 -- 3 --'

Where E is a merge commit that bring the feature branch back into master, rather than:

master: A --- B --- C --- D --- 1 --- 2 --- 3 --- F
                               ^-was feature-^

Where the feature branch is rebased and a fast-forward merge brings those commits into master ?

[mdlinville]

It doesn't work for documentation workflows where two different branches are moving forward at one time, and branch B needs to have all the work from branch A underneath all of branch B's work at all times. Kubernetes docs are continuously improved and published, in a separate stream from not-yet-published features. The stream of not-yet-published features also needs the continuous improvements to go underneath it. In sort, docs work differently from code, unless you want us to hold all continuous improvement and publish it simultaneous with a new Kubernetes release.

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[fejta-bot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

[fejta-bot]

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

[k8s-ci-robot]

@fejta-bot: Closing this issue.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.