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

Fix third party #25894

Merged
merged 1 commit into from
Jun 4, 2016
Merged

Fix third party #25894

merged 1 commit into from
Jun 4, 2016
+163 −37

Conversation

brendandburns
Copy link
Contributor

@brendandburns brendandburns commented May 19, 2016 •
edited
Loading

Fixes #25421
Fixes #25422

@adohe @sjenning @caesarxuchao @lavalamp

@kubernetes/sig-api-machinery

Analytics

@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed labels May 19, 2016
@brendandburns brendandburns added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label May 19, 2016
@brendandburns brendandburns added this to the v1.3 milestone May 19, 2016
@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 May 19, 2016
@lavalamp lavalamp assigned lavalamp and krousey and unassigned bgrant0607 and lavalamp May 19, 2016
@lavalamp
Copy link
Member

@krousey can you review this? I'm buried today.

@brendandburns
Copy link
Contributor Author

(I'm fixing this unit test...)

@brendandburns
Copy link
Contributor Author

@k8s-bot test this please issue: #25925

@brendandburns
Copy link
Contributor Author

@k8s-bot unit test this please issue: #25925

@krousey
Copy link
Contributor

krousey commented May 23, 2016

@brendandburns Are you looking to get this in today? It seems there are some interaction issues with the proto encoder.

@smarterclayton
Copy link
Contributor 

�(BI0520 13:45:20.316557    2956 kubelet.go:2497] skipping pod synchronization - [Failed to start ContainerManager system validation failed - Following Cgroup subsystem not mounted: [memory]]

@brendandburns
Copy link
Contributor Author

I sorted out at least one problem, still looking into the e2e...

@brendandburns brendandburns force-pushed the thirdparty-watch branch 4 times, most recently from b4d3ced to 7494fcb Compare May 26, 2016 22:27
@brendandburns
Copy link
Contributor Author

@k8s-bot node e2e test this issue: #26385

@brendandburns
Copy link
Contributor Author

@krousey I believe this is ready for review, node e2e is a flake.

@krousey
Copy link
Contributor

krousey commented May 27, 2016

@brendanburns What was the issue earlier?

@brendandburns
Copy link
Contributor Author

brendandburns commented May 27, 2016 •
edited
Loading

There were two issues:

  1. I wasn't handling YAML properly in the IsThirdPartyResource function

  2. I wasn't properly determining if an object was third party if the into object was a VersionedObjects and the GroupVersionKind was nil

@krousey
Copy link
Contributor

krousey commented May 27, 2016

This is LGTM, but should there be test cases at least for the VersionedObjects case?

@brendandburns brendandburns added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels May 27, 2016
@sdminonne
Copy link
Contributor

/sub

@krousey
Copy link
Contributor

krousey commented Jun 1, 2016

@brendanburns Friendly ping. Is that a no on the test case?

@brendandburns brendandburns force-pushed the thirdparty-watch branch 2 times, most recently from 5b1efdb to 86baffc Compare June 2, 2016 17:13
@brendandburns
Copy link
Contributor Author

@krousey that's a "it was memorial day weekend" ;)

Test case added, please take another look.

@krousey krousey added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 2, 2016
@brendandburns brendandburns mentioned this pull request Jun 4, 2016
Closed
@brendandburns
Copy link
Contributor Author

@k8s-bot unit test this please issue: #23879

@k8s-bot
Copy link

k8s-bot commented Jun 4, 2016

GCE e2e build/test passed for commit 328a839.

@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 Jun 4, 2016

GCE e2e build/test passed for commit 328a839.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 421c16a into kubernetes:master Jun 4, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@krousey krousey

Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.3
Development

Successfully merging this pull request may close these issues.
9 participants
@brendandburns @lavalamp @krousey @smarterclayton @sdminonne @k8s-bot @k8s-github-robot @googlebot @bgrant0607