Development workflow for Tekton

[afrittoli]

The GitHub workflow we use today can be painful when reviewing PRs.

Here some thoughts:

• Reviewing new revisions of a PR can be really hard as we cannot see the diff between revision. Old revisions are overwritten via git push --force to the fork
• GitHub provides a “show changes since your last review” function, but it doesn’t work if we override commits
• Having contributors maintaining a single commit per PR helps to have a nice and clean github history, however when things changes along the way, often the PR description is updated, but the commit message may stay the same, and GitHub does not really help putting the commit message as part of the review (it’s “hidden” in a dedicated tab

So perhaps we could add an extra workflow option where:

• We add commits for new revisions
• We ask users to squash commits before merge and review the commit message

There are a few downsides to this:

• Changing process implies an extra effort for contributors to adapt and a transition phase with some confusion that will happen.
• If we support multiple workflows on the long run, this may create confusion. This could be alleviated by migrating to the new workflow for all changes.
• The new workflow requires an extra step for contributors to squash commits, which might be painful is the PR has an lgtm already, since the lgtm will go away. The pain here could be alleviated by using "approve" first, and wait for the squash before the final lgtm.

We could have tide squash commits on behalf of the user, however tide will take the PR title as final commit message, with goes against out commit message best practices. We could extend this mechanism or replace tide with our own merge service, but this would require a significant amount of development work.

[afrittoli]

/cc @tektoncd/core-collaborators

[vdemeester]

Yeah this is a tricky one and I don't think we can satisfy everybody even. I honestly do think, given the downside, … that you wrote, that, at lest as far as I am concerned, the current approach is good enough (or less worse than others). The GitHub limitiations are.. limitations but even, if we manage to keep PRs to decent size, re-reviewing the whole diff is not a big deal for me. I tend to only review the whole diff each time because it helps me make sure I am reviewing what is gonna get in (and not part of it).

We do already ask user to squash commit before merge if there is too much commits or commits that are like "review comment 1". And some users will add commits for new revision, and wait for several "yes"/lgtm before squashing, others (like me), will heavily user rebase and --force-with-lease push. The current flow works for both those so, why change ? 😛

[afrittoli]

Re-reviewing everything does not work so well for me for larger PRs.
I think it's more error-prone since I've seen the code already, so my eyes are more likely to miss things as mis-take a change for something that I've reviewed already.

I do agree that we can support today (with no infra changes) the approach with multiple commits, but I think it would be valuable to at least call-out the two workflows in our docs, and perhaps set some rules, for instance "use multiple commits when the PR is labelled as XL or more"

[AlanGreene]

Some interesting changes on the GitHub roadmap that look like they could help: https://github.com/github/roadmap/issues/211

In particular I'm pretty excited about this part since I have a similar workflow to @vdemeester, heavily using rebase / force push:

Revisions are persisted across force pushes

[afrittoli]

Heh, interesting, Gerrit like revisions through forced pushes. I agree it would help, once it's implemented.