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

JSON marshaling/unmarshaling of label Selector #17217

Merged
merged 1 commit into from
Nov 21, 2015
Merged

JSON marshaling/unmarshaling of label Selector #17217

merged 1 commit into from
Nov 21, 2015
+211 −0

Conversation

wojtek-t
Copy link
Member

@k8s-github-robot
Copy link

Labelling this PR as size/M

@k8s-github-robot k8s-github-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Nov 13, 2015
@k8s-bot
Copy link

k8s-bot commented Nov 13, 2015

GCE e2e test build/test passed for commit 748a639e5b8057e466b830aefac0ad25bc037ced.

@wojtek-t wojtek-t changed the title [WIP] JSON marshaling/unmarshaling of label Selector JSON marshaling/unmarshaling of label Selector Nov 13, 2015
@k8s-github-robot
Copy link

Labelling this PR as size/L

@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 13, 2015
@k8s-bot
Copy link

k8s-bot commented Nov 13, 2015

GCE e2e test build/test passed for commit 9de5bdc51d2b6d010ad082d46066130ec6e66e88.

@wojtek-t
Copy link
Member Author

@lavalamp - PTAL

@wojtek-t wojtek-t mentioned this pull request Nov 16, 2015
Merged
lavalamp
lavalamp reviewed Nov 17, 2015
View reviewed changes
pkg/fields/selector.go
@@ -235,3 +236,26 @@ func parseSelector(selector string, fn TransformFunc) (Selector, error) {
func OneTermEqualSelector(k, v string) Selector {
return &hasTerm{field: k, value: v}
}

// SelectorHolder is a wrapper around Selector that allows for
Copy link
Member

Choose a reason for hiding this comment

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

I was sure I'd already left a comment here, but I don't see it now. Maybe it's sitting unsent in one of my many open tabs.

Anyway, what do you think about moving these holders to pkg/api/unversioned, where we have other similar types?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@wojtek-t wojtek-t force-pushed the marshall_selector branch 2 times, most recently from 73f07c9 to 2f84014 Compare November 18, 2015 10:27
@k8s-bot
Copy link

k8s-bot commented Nov 18, 2015

GCE e2e build/test failed for commit 73f07c9ceadda964718521b62ec26645bf0bc722.

@k8s-bot
Copy link

k8s-bot commented Nov 18, 2015

GCE e2e build/test failed for commit 2f84014ac29951a0311bdcbcb863dc6202168bc8.

@lavalamp
Copy link
Member

LGTM

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

If you make a follow-up, I recommend changing the name to drop the "Holder" suffix on the type names-- it's not needed any more.

@lavalamp
Copy link
Member

@k8s-bot test this

@k8s-bot
Copy link

k8s-bot commented Nov 18, 2015

GCE e2e build/test failed for commit 2f84014ac29951a0311bdcbcb863dc6202168bc8.

@k8s-bot
Copy link

k8s-bot commented Nov 18, 2015

GCE e2e build/test failed for commit 2f84014ac29951a0311bdcbcb863dc6202168bc8.

@wojtek-t
Copy link
Member Author

@k8s-bot e2e test this please

@k8s-bot
Copy link

k8s-bot commented Nov 19, 2015

GCE e2e test build/test passed for commit 2f84014ac29951a0311bdcbcb863dc6202168bc8.

@wojtek-t wojtek-t 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 19, 2015
@wojtek-t
Copy link
Member Author

I've just done the rename requested above.

@k8s-bot
Copy link

k8s-bot commented Nov 19, 2015

GCE e2e test build/test passed for commit 975871e.

@lavalamp
Copy link
Member

Thanks! still LGTM

@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 test build/test passed for commit 975871e.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

k8s-github-robot pushed a commit that referenced this pull request Nov 21, 2015
Merge pull request #17217 from wojtek-t/marshall_selector
42fcaf0 
Auto commit by PR queue bot
@k8s-github-robot k8s-github-robot merged commit 42fcaf0 into kubernetes:master Nov 21, 2015
@wojtek-t wojtek-t deleted the marshall_selector branch November 23, 2015 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@lavalamp lavalamp

Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@wojtek-t @k8s-github-robot @k8s-bot @lavalamp @k8s-teamcity-mesosphere @googlebot