Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Discussion: switch to merge workflow #11040

Open
VannTen opened this issue Mar 28, 2024 · 8 comments
Open

Discussion: switch to merge workflow #11040

VannTen opened this issue Mar 28, 2024 · 8 comments
Labels
lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. tide/merge-method-rebase Denotes a PR that should be rebased by tide when it merges.

Comments

@VannTen
Copy link
Contributor

VannTen commented Mar 28, 2024

Currently, we are using the "squash" method from tide (the merger component of Prow)

This pose some problems:

  • Dependents PRs (PRs with a starting point being the tip of another PR, useful when something require a lot of refactoring / loosely related changes, see Migrate DNS manifests to kubernetes.core.k8s #10701 for instance ) have to be "manually" rebased on top of master once the prerequisite PR is merged.
    ( By manually, I mean that git rebase is usually able to detect "already applied commits", but squashing breaks that.)
  • Squash throw away history granularity. When doing a git blame (which I do fairly often these days when trying to refactor stuff which seems strange), I have often unrelated changes mashed together, which make it harder to understand the point of the line I'm looking for.
  • Commits messages matters, or at least they should. Squashing create a bad incentive to just not care about them, they'll be squashed together anyway and can't explain the change associated with them. (Since you can't associate a message with a precise set of changes)

Cons of a merge workflow:
The only arguments I usually here are:

  • squash make for a nice (hear: linear) history -> besides the aesthetic, I don't see what practical benefits this brings
  • noise commits (aka typo fixes, etc) -> this is the job of the PR author to rebase its branch before merging, if necessary, arranging commits in logical chunks. (and of the reviewers to not accept badly written history)

Soliciting advice from @MrFreezeex @mzaian @floryut @yankay (and anyone else)

@nicolas-goudry
Copy link
Contributor

I’ll allow myself to give my thoughts about this.

The only thing that I’m concerned about is that doing this would give more work to reviewers, since I’m pretty sure that most authors won’t bother cleaning their commits before opening a PR. Maybe this is acceptable, I just wanted to point it out.

Other than that concern, I strongly agree with all your points and think that this is a very good idea.

@VannTen
Copy link
Contributor Author

VannTen commented Mar 28, 2024

We should actually be doing this now, but we don't exactly bother, and I think the squash workflow is one of the reasons.
The work is not in vain though, because otherwise you have to do more work when refactoring/understanding the code/etc (and I think there is often an intersection between people doing refactors and reviewers)

@champtar
Copy link
Contributor

FYI anyone you can already use

/label tide/merge-method-rebase

@k8s-ci-robot k8s-ci-robot added the tide/merge-method-rebase Denotes a PR that should be rebased by tide when it merges. label Mar 28, 2024
@VannTen
Copy link
Contributor Author

VannTen commented Mar 29, 2024 via email

@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue as fresh with /remove-lifecycle stale
  • Close this issue with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 27, 2024
@nicolas-goudry
Copy link
Contributor

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 27, 2024
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue as fresh with /remove-lifecycle stale
  • Close this issue with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 25, 2024
@VannTen
Copy link
Contributor Author

VannTen commented Sep 25, 2024 via email

@k8s-ci-robot k8s-ci-robot added lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Sep 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. tide/merge-method-rebase Denotes a PR that should be rebased by tide when it merges.
Projects
None yet
Development

No branches or pull requests

5 participants