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

Update ObjectTyper for GroupVersion #17796

Merged
merged 1 commit into from
Dec 7, 2015
Merged

Update ObjectTyper for GroupVersion #17796

merged 1 commit into from
Dec 7, 2015
+338 −273

Conversation

deads2k
Copy link
Contributor

@deads2k deads2k commented Nov 25, 2015

Makes ObjectTyper use GroupVersionKind.

ref #17216

@liggitt @caesarxuchao

@k8s-github-robot
Copy link

Labelling this PR as size/XL

@k8s-github-robot k8s-github-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Nov 25, 2015
@k8s-bot
Copy link

k8s-bot commented Nov 25, 2015

GCE e2e test build/test passed for commit 496b7b166c212bfaf50340c00506d1a3ad4e7ff7.

@deads2k deads2k mentioned this pull request Nov 25, 2015
Closed
61 tasks
@k8s-bot
Copy link

k8s-bot commented Nov 25, 2015

GCE e2e test build/test passed for commit e8b3b9d19c9257f9cd7cc57405b84f259b838328.

derekwaynecarr
derekwaynecarr reviewed Nov 26, 2015
View reviewed changes
pkg/api/serialization_test.go
@@ -57,6 +57,9 @@ func fuzzInternalObject(t *testing.T, forVersion string, item runtime.Object, se
func roundTrip(t *testing.T, codec runtime.Codec, item runtime.Object) {
printer := spew.ConfigState{DisableMethods: true}

gvk, err := api.Scheme.ObjectKind(item)
t.Logf("fqKind for %v is %v with codec %v", reflect.TypeOf(item), gvk, codec)
Copy link
Member

Choose a reason for hiding this comment

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

not sure if fqKind is used elsewhere, but prefer "fully qualified kind"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure if fqKind is used elsewhere, but prefer "fully qualified kind"

As the message or as the variable name? fullyQualifiedKind seems a bit long, I was hoping for fqKind as a compromise

@deads2k
Copy link
Contributor Author

deads2k commented Nov 26, 2015

@k8s-bot unit test this please

@deads2k
Copy link
Contributor Author

deads2k commented Nov 30, 2015

@eparis is the permanent yellow a bot bug or is it intentional?

@deads2k deads2k mentioned this pull request Dec 1, 2015
Merged
liggitt
liggitt reviewed Dec 1, 2015
View reviewed changes
pkg/api/serialization_test.go
}
// if we don't recognize the type, then we'll never be able to round trip it
if !api.Scheme.Recognizes(gv.WithKind(gvk.Kind)) {
return
Copy link
Member

Choose a reason for hiding this comment

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

no error in this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no error in this case?

This matches the set check below. Basically, they run the same unit tests multiple times and switch group,versions each time, so the test can be run sometimes and not others. This is the correct form. Refactoring its location doesn't work out cleaner because it makes the structure mix and match.

@derekwaynecarr
Copy link
Member

@liggitt - is not feeling well.

I will take over reviewing this PR in his absence.

@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 2, 2015
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 3, 2015
@k8s-bot
Copy link

k8s-bot commented Dec 3, 2015

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

@derekwaynecarr
Copy link
Member

LGTM

@derekwaynecarr derekwaynecarr added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 3, 2015
@k8s-github-robot
Copy link

Travis continuous integration appears to have missed, closing and re-opening to trigger it

@k8s-github-robot
Copy link

@k8s-bot test this

Tests are more than 48 hours old. Re-running tests.

@k8s-bot
Copy link

k8s-bot commented Dec 5, 2015

GCE e2e build/test failed for commit 2d6850688c74b481d61b78f0f9848f137a4c3185.

@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 6, 2015
@k8s-github-robot
Copy link

PR changed after LGTM, removing LGTM.

@k8s-github-robot k8s-github-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Dec 7, 2015
@deads2k deads2k added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 7, 2015
@k8s-bot
Copy link

k8s-bot commented Dec 7, 2015

GCE e2e build/test failed for commit f764e00.

@deads2k
Copy link
Contributor Author

deads2k commented Dec 7, 2015

@k8s-bot test this please

@k8s-bot
Copy link

k8s-bot commented Dec 7, 2015

GCE e2e build/test failed for commit f764e00.

@deads2k
Copy link
Contributor Author

deads2k commented Dec 7, 2015

out of disk

@k8s-bot test this please

@k8s-bot
Copy link

k8s-bot commented Dec 7, 2015

GCE e2e test build/test passed for commit f764e00.

@deads2k
Copy link
Contributor Author

deads2k commented Dec 7, 2015

@k8s-bot unit test this please

@deads2k
Copy link
Contributor Author

deads2k commented Dec 7, 2015

@k8s-bot test this

@deads2k
Copy link
Contributor Author

deads2k commented Dec 7, 2015

failure to checkout

@k8s-bot unit test this

@k8s-bot
Copy link

k8s-bot commented Dec 7, 2015

GCE e2e test build/test passed for commit f764e00.

@deads2k
Copy link
Contributor Author

deads2k commented Dec 7, 2015

timeout (worked before)

@k8s-bot unit test this

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

GCE e2e test build/test passed for commit f764e00.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

k8s-github-robot pushed a commit that referenced this pull request Dec 7, 2015
Merge pull request #17796 from deads2k/gv-object-typer
ec1ba74 
Auto commit by PR queue bot
@k8s-github-robot k8s-github-robot merged commit ec1ba74 into kubernetes:master Dec 7, 2015
mikedanese
mikedanese reviewed Dec 11, 2015
View reviewed changes
pkg/conversion/scheme.go
// if err := s.SetVersionAndKind(outVersion, outKind.Kind, out); err != nil {
// return nil, err
// }
// =======
Copy link
Member

Choose a reason for hiding this comment

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

??? what's this stuff

@deads2k deads2k deleted the gv-object-typer branch February 26, 2016 18:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@derekwaynecarr derekwaynecarr

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

Successfully merging this pull request may close these issues.
9 participants
@deads2k @k8s-github-robot @k8s-bot @derekwaynecarr @liggitt @smarterclayton @mikedanese @k8s-teamcity-mesosphere @googlebot