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

Notes regarding "Require branches to be up to date before merging" #445

Closed
petertseng opened this issue Nov 28, 2016 · 7 comments
Closed

Comments

@petertseng
Copy link
Member

They're in #444

I just created this issue to help me find them easier.

If we revert it:

For the situation of "Branches A and B individually pass the tests, but together they would fail", the procedure of:

  • Merge A
  • Click "Restart build" on B

will then cause the Travis CI check for B to fail (as it should), but the disadvantage is we'd have to remember to click that "Restart build" button, and we might forget (because we are all human).

So we will leave it as is for now!

I don't need comments on this issue, I'm going to close it immediately.

I think I would like an email for this though, so maybe someone could comment on it and then delete their comment (if necessary)?

@rbasso
Copy link
Contributor

rbasso commented Nov 28, 2016

Commenting just to notify @petertseng.

@petertseng
Copy link
Member Author

petertseng commented May 8, 2017

I got really annoyed that I created 6 PRs in a row (#525, #526, #527, #528, #529, #530), but the "Require branches to be up to date before merging" would have forced me to merge one, update the next one, wait 5 minutes for build to pass, and so on, taking 25+ minutes to merge the PRs. After merging the first two, I got fed up and just octopus merged the other four.

381d339

command-line only, and you have to make Travis build that commit on a branch before it can be pushed to master, but at least it's better than waiting 5 minutes per PR.

@petertseng
Copy link
Member Author

That means ultimately I don't need this setting changed, since I (finally!) found a way to fit it into my workflow (octopus merge) that doesn't make me go insane.

However, if octopus merges make anyone other than me go insane, then the only way to resolve our differences may be in fact to change that setting. Because I will go insane without octopus merges.

petertseng referenced this issue May 8, 2017
change 1.0.0.2: add cases from x-common
run-length-encoding 1.0.0.2: add whitespace and lowercase
largest-series-product 1.0.0.2: make simple case first, rm largest
saddle-points 1.0.0.2: follow x-common
@rbasso
Copy link
Contributor

rbasso commented May 8, 2017

Ideally, this setting makes sense, but I also think that it is annoying.

The probability of having commits that pass individual tests but fail the combined ones is remote and would be detected in the next PR. So it seems that there is almost no risk in disabling it.

Feel free to ask Katrina to disable it. 👍

@petertseng
Copy link
Member Author

Hmm, now I'm not sure how I got the octomerge to work last time. I'll keep trying a bit but eventually I may have to merge without it if I can't figure it out.

@petertseng
Copy link
Member Author

petertseng commented May 9, 2017

Okay that worked... variables to check:

I was unsure because originally the merge got rejected, but I couldn't tell which of the two factors it was.

@petertseng
Copy link
Member Author

Historical notes: Octomerges performed in this repo:

I wrote down the former, but not the latter.

petertseng referenced this issue Sep 25, 2017
bowling 1.0.0.2: drop unneeded rolls, edit a description
leap 1.0.0.2: explain criteria in descriptions, put in progress order
luhn 1.0.0.2: only test isValid, use (most) x-common cases
phone-number 1.2.0.3: enforce area/exchange not starting with 1
roman-numerals 1.0.0.2: add descriptions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants