Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

[Proposal] Rebase on top of master before running build #1947

Closed
wants to merge 1 commit into from

Conversation

hiddeco
Copy link
Member

@hiddeco hiddeco commented Apr 16, 2019

This prevents scenarios where builds would succeed for pull requests
but failed after merging them into master, due to e.g. dependency
changes or code that looked auto mergable but wasn't.

The trade-off is that PR builds will fail faster / untimely, local
builds will however still succeed so people can rebase at their own
pace when they are ready for it.

@hiddeco hiddeco added the build About the build or test scaffolding label Apr 16, 2019
@hiddeco hiddeco force-pushed the build/rebase-master branch 4 times, most recently from 1243edf to e28267f Compare April 18, 2019 13:33
This prevents scenarios where builds would succeed for pull requests
but failed after merging them into master, due to e.g. dependency
changes or code that looked auto mergable but wasn't.

The trade-off is that PR builds will fail faster / untimely, local
builds will however still succeed so people can rebase at their own
pace when they are ready for it.
@2opremio
Copy link
Contributor

I have mixed feelings about this. What would happen if there is a conflict? Things won't be tested at all, right?

getting the e2e tests to work with my branch (with conflicts) it's valuable before rebasing.

I think I would test both things, before and after rebasing. Or, if testing both things is overkill, maybe we can live with the testing errors due to deviations from time to time.

@hiddeco
Copy link
Member Author

hiddeco commented Apr 18, 2019

What would happen if there is a conflict?

It would fail so you are forced to rebase.

Getting the e2e tests to work with my branch (with conflicts) it's valuable before rebasing.

That would be an option.

Another option I have would be to "require branches to be up to date before merging", which you can enable in the protected branch settings of the repository. This would however force people to always rebase and disable any automagic merge strategies git can now perform.

@2opremio
Copy link
Contributor

2opremio commented Apr 18, 2019

Another option I have would be to "require branches to be up to date before merging"

I like this, it has the advantage of both worlds, and rebasing shouldn't be hassle.

This would however force people to always rebase and disable any automagic merge strategies git can now perform.

We can do them locally, can't we?

@squaremo
Copy link
Member

We can use branch protection to make sure PRs are up-to-date (fast-forwardable on the target branch) before merging. That means CI will run against the branch as it is, but the PR will not be mergeable until CI passes on it and it is up to date with respect to its target (e.g., master).

@hiddeco
Copy link
Member Author

hiddeco commented Apr 25, 2019

We can use branch protection to make sure PRs are up-to-date (fast-forwardable on the target branch) before merging. That means CI will run against the branch as it is, but the PR will not be mergeable until CI passes on it and it is up to date with respect to its target (e.g., master).

Is what I just enabled, we will see how this goes.

@hiddeco hiddeco closed this Apr 25, 2019
@2opremio 2opremio deleted the build/rebase-master branch December 12, 2019 13:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
build About the build or test scaffolding
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants