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

Validation cleanup part 1 #17245

Merged
merged 5 commits into from
Nov 23, 2015

Conversation

thockin
Copy link
Member

@thockin thockin commented Nov 13, 2015

Part 1 of several. Merge fielderrors and validation together, simplify names.

@lavalamp

@k8s-github-robot
Copy link

Labelling this PR as size/XXL

@k8s-github-robot k8s-github-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Nov 13, 2015
@@ -1,207 +0,0 @@
/*
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is way too much of a friday for me to review in depth, but I thought these were here to break some cycle? Maybe not. I like where you put it, though I would consider a sub-package there, too.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No cycle, and one of the followup PRs does in fact create a sub-pkg :)

No sweat that it is friday - I wanted to get these off my laptop before I broke it.

@k8s-bot
Copy link

k8s-bot commented Nov 13, 2015

GCE e2e build/test failed for commit 40cd8851f91c45f90c5780d5516bcf4571f9ad32.

@thockin
Copy link
Member Author

thockin commented Nov 14, 2015

unit test failed for no reason:

No changes. 
GitHub pull request #17245 of commit 40cd8851f91c45f90c5780d5516bcf4571f9ad32, no merge conflicts.
Test Result (no failures)

@thockin
Copy link
Member Author

thockin commented Nov 14, 2015

@k8s-bot unit test this please

@k8s-bot test this please

@k8s-bot
Copy link

k8s-bot commented Nov 14, 2015

GCE e2e build/test failed for commit 40cd8851f91c45f90c5780d5516bcf4571f9ad32.

@thockin
Copy link
Member Author

thockin commented Nov 14, 2015

@k8s-bot unit test this please

@k8s-bot test this please

@k8s-bot
Copy link

k8s-bot commented Nov 14, 2015

GCE e2e build/test failed for commit 40cd8851f91c45f90c5780d5516bcf4571f9ad32.

@k8s-bot
Copy link

k8s-bot commented Nov 14, 2015

GCE e2e build/test failed for commit 636ab2f7042acc2a7b7039eb377e4502b4cf5206.

@k8s-bot
Copy link

k8s-bot commented Nov 14, 2015

GCE e2e build/test failed for commit 19b23d24da6ed716fbd9c6bc9306674ba51c458c.

@thockin
Copy link
Member Author

thockin commented Nov 14, 2015

@k8s-bot test this please

@k8s-bot
Copy link

k8s-bot commented Nov 14, 2015

GCE e2e test build/test passed for commit 636ab2f7042acc2a7b7039eb377e4502b4cf5206.

@bgrant0607 bgrant0607 assigned lavalamp and unassigned bgrant0607 Nov 14, 2015
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 16, 2015
@thockin thockin removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 16, 2015
@thockin
Copy link
Member Author

thockin commented Nov 16, 2015

I have rebased this one, but this is going to be a race. If there's anything I can do to make reviews on this sequence of PRs easier, please holler

@k8s-bot
Copy link

k8s-bot commented Nov 16, 2015

GCE e2e test build/test passed for commit 4457a660d8b6c59429a105d6413e48c90c310b81.

@lavalamp lavalamp added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 16, 2015
@lavalamp
Copy link
Member

LGTM - I would ask for a squash but I know that might make a rebase harder, should you need to do one-- your call

@thockin
Copy link
Member Author

thockin commented Nov 16, 2015

I kept these commits distinct on putrpose. I can do a squash if you want, but I think it is better without

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 17, 2015
@wojtek-t
Copy link
Member

@thockin - I'm afraid you need to rebase :(

@thockin
Copy link
Member Author

thockin commented Nov 19, 2015

This is painful.

How do I run the munger by hand? I tried:

thockin@freakshow:~/src/contrib/mungegithub$ ./mungegithub --dry-run
I1119 13:21:12.410656   21923 github.go:188] Made 0 API calls since the last Reset 0.000000 calls/sec
I1119 13:21:12.806775   21923 mungegithub.go:54] Running mungers
E1119 13:21:12.807053   21923 github.go:63] Ran out of github API tokens. Sleeping for 39.386549198666664 minutes

@k8s-oncall, I'm gonna merge it later today if this isn't unstuck

@eparis
Copy link
Contributor

eparis commented Nov 19, 2015

you basically MUST use --token-file or --token

./mungegithub --token-file=/home/eparis/github-oauth --dry-run --token-file=/home/eparis/github-oauth --v=10 --max-pr-number=17245 --min-pr-number=17245 --period=2m -pr-mungers=submit-queue

@lavalamp
Copy link
Member

This is currently 11th or so in the queue.

@eparis
Copy link
Contributor

eparis commented Nov 19, 2015

when we get this far behind I almost want to run 2 merges at the same time. I know it could give us non-linear/obvious e2e failures though :-(

@thockin
Copy link
Member Author

thockin commented Nov 19, 2015

Could we do 2-3 merges per run?

On Thu, Nov 19, 2015 at 1:59 PM, Eric Paris notifications@github.com
wrote:

when we get this far behind I almost want to run 2 merges at the same
time. I know it could give us non-linear/obvious e2e failures though :-(


Reply to this email directly or view it on GitHub
#17245 (comment)
.

@eparis
Copy link
Contributor

eparis commented Nov 19, 2015

We merge as fast as we can run e2e. e2e takes like 20-30 minutes I think. So we only merge 2-3 per hour. It would be possible to run the e2e of 2 PRs at the same time but then you aren't getting good results when the second one merges. If PR A and PR B both passed individually but together they failed we wouldn't detect an e2e failure until PR C.

@thockin
Copy link
Member Author

thockin commented Nov 19, 2015

I meant to run e2e on master+A+B and if it passes, merge A and B. If A+B fails, try A alone. if we did this 2 or three at a time we could double our throughput with only a little more risk (as long as nobody hand-merges)

@thockin
Copy link
Member Author

thockin commented Nov 19, 2015

@lavalamp where do you see the queue? I still do not see that PR in http://submit-queue.k8s.io/

@eparis
Copy link
Contributor

eparis commented Nov 19, 2015

The queue just stopped because google internal e2e just failed: kubernetes-e2e-gke-ci

I understand what you mean now, but I'd have to leave that implementation to someone who can make changes or provide a better/different interface to Jenkins GCE e2e

@eparis
Copy link
Contributor

eparis commented Nov 19, 2015

The queue is tab 2 called 'Github E2E' Maybe it should be renamed.

@lavalamp
Copy link
Member

Yeah, I have so far today filed (or bumped priority to P0) on 5 different flaky test bugs.

@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link

k8s-bot commented Nov 21, 2015

GCE e2e build/test failed for commit 3464e504a66e1d3bb3bef59e5ad867ef2da6a786.

@eparis
Copy link
Contributor

eparis commented Nov 21, 2015

Might be a real e2e failure.

@thockin
Copy link
Member Author

thockin commented Nov 23, 2015

rebased again

@thockin thockin added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Nov 23, 2015
@k8s-bot
Copy link

k8s-bot commented Nov 23, 2015

GCE e2e test build/test passed for commit bca3780.

@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link

k8s-bot commented Nov 23, 2015

GCE e2e test build/test passed for commit bca3780.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

k8s-github-robot pushed a commit that referenced this pull request Nov 23, 2015
@k8s-github-robot k8s-github-robot merged commit 32c240b into kubernetes:master Nov 23, 2015
@thockin thockin deleted the airplane_validation_pt1 branch December 14, 2015 16:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants