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.

Sign up for GitHub

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

Jump to bottom

Validation cleanup parts 5 & 6 together #18276

Merged
merged 3 commits into from
Dec 11, 2015
Merged

Validation cleanup parts 5 & 6 together #18276

merged 3 commits into from
Dec 11, 2015

Conversation

thockin
Copy link
Member

@thockin thockin commented Dec 7, 2015

Part 6 of several

Make a sub-pkg and shorter names for better readability
Some errors that happen during validation are not validation errors.

@lavalamp

@thockin thockin changed the title Validation cleanup part 6 Validation cleanup parts 5 & 6 together Dec 7, 2015
@thockin
Copy link
Member Author

thockin commented Dec 7, 2015

This is the last validation PR that I have pending (though I have more that are half-cooked).

@k8s-bot
Copy link

k8s-bot commented Dec 7, 2015

GCE e2e build/test failed for commit a04f350fc1a52a59cbf03e76512647b9784da403.

@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 Dec 7, 2015
@thockin thockin assigned lavalamp and unassigned bgrant0607 Dec 7, 2015
@thockin thockin force-pushed the airplane_validation_pt6 branch from a04f350 to a879d7e Compare December 7, 2015 18:29
@k8s-bot
Copy link

k8s-bot commented Dec 7, 2015

GCE e2e test build/test passed for commit a879d7eff3314b3bed65880b9a4a35cfd3b20280.

@thockin
Copy link
Member Author

thockin commented Dec 9, 2015

I'm happy to reassign this - I know you are leaving on vaca soon

@lavalamp
Copy link
Member

lavalamp commented Dec 9, 2015

Yeah, I'm really sorry-- the past few days I've been continually opening this up and then not actually reviewing it, I did it again like five minutes ago :(

@thockin thockin assigned caesarxuchao and unassigned lavalamp Dec 9, 2015
@thockin
Copy link
Member Author

thockin commented Dec 9, 2015

Shooting randomly into the crowd, I hit @caesarxuchao

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 9, 2015
caesarxuchao
caesarxuchao reviewed Dec 10, 2015
View reviewed changes
pkg/registry/service/rest.go
return nil, errors.NewInvalid("Service", service.Name, el)
// TODO: what error should be returned here? It's not a
// field-level validation failure (the field is valid), and it's
// not really an internal error.
Copy link
Member

Choose a reason for hiding this comment

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

It seems the only possible cause of error here is the ports pool is depleted, so how about a 503 Service Unavailable?

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't want to change behavior in this PR, just cleanup. That's a good answer for a different PR :)

@caesarxuchao
Copy link
Member

LGTM except a nit.

@thockin thockin force-pushed the airplane_validation_pt6 branch from a879d7e to 8e43523 Compare December 10, 2015 19:45
@thockin
Copy link
Member Author

thockin commented Dec 10, 2015

Rebasing now

@thockin thockin force-pushed the airplane_validation_pt6 branch from 8e43523 to 872faff Compare December 10, 2015 19:48
@k8s-bot
Copy link

k8s-bot commented Dec 10, 2015

GCE e2e build/test failed for commit 8e43523fe198731f0e8cd45560ce00ecd2bedb7d.

@caesarxuchao
Copy link
Member

Thanks. LGTM.

@caesarxuchao caesarxuchao added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed cla: yes labels Dec 10, 2015
@caesarxuchao caesarxuchao added cla: yes and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Dec 10, 2015
@k8s-bot
Copy link

k8s-bot commented Dec 10, 2015

GCE e2e test build/test passed for commit 872faff.

@thockin
Copy link
Member Author

thockin commented Dec 10, 2015

@k8s-bot test this please

@k8s-bot
Copy link

k8s-bot commented Dec 10, 2015

GCE e2e test build/test passed for commit 872faff.

@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 Dec 11, 2015

GCE e2e test build/test passed for commit 872faff.

@thockin
Copy link
Member Author

thockin commented Dec 11, 2015

@k8s-bot unit test this please

@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 Dec 11, 2015

GCE e2e test build/test passed for commit 872faff.

j3ffml added a commit that referenced this pull request Dec 11, 2015
@j3ffml
Merge pull request #18276 from thockin/airplane_validation_pt6
9c49cda 
Validation cleanup parts 5 & 6 together
@j3ffml j3ffml merged commit 9c49cda into kubernetes:master Dec 11, 2015
@thockin thockin deleted the airplane_validation_pt6 branch December 14, 2015 16:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@caesarxuchao caesarxuchao

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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@thockin @k8s-bot @k8s-github-robot @lavalamp @caesarxuchao @k8s-teamcity-mesosphere @j3ffml @googlebot @bgrant0607