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

Client-go migration #1638

Merged
merged 8 commits into from
Feb 16, 2017
Merged

Conversation

floreks
Copy link
Member

@floreks floreks commented Feb 14, 2017

This is a big change and extensive tests are needed. It will take some time to review and I can finish what's left in the meantime.

What's done

  • Client-go - updated to current master, commit hash: 86a2be1b447d7abddae88de0a5f642935992b803
  • 90% of the code migrated to use client-go instead of kubernetes deps
  • Fixed most backend tests
  • Cleaned up kubernetes dependencies. Left only the one needed for deploy from file (kubectl utils) and updated, commit hash: 871fc426b3576bfe9a92b3ca1d4bfa83ea71edd5

TODO

  • Get rid of kubernetes dependencies
  • Fix 10 tests that are skipped
  • Fix deployment detail and not use deployment utils from kubernetes
  • Fix deployment deploy not to use kubectl utils
  • Enable endpoint for getting deployments old replica sets

Dashboard is running and travis should be green, however 1 endpoint is disabled and 10 tests are skipped.

In order to get rid of kubernetes as dependency and fully migrate to client-go we'd need to rewrite some code. Rewriting deployment details and getting replica sets should not be hard but getting rid of kubectl as dependency might be harder. If we want to do that then I will create issues for that.

I have not updated kubernetes dependencies because current master and 1.6.0-alpha are broken. Same dependency error as in client-go.

Closes #1457

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Feb 14, 2017
@rf232
Copy link
Contributor

rf232 commented Feb 14, 2017

Awesome, current changes look good (apart from a run of gofmt on the test code.)

In a way I'm happy that you haven't fixed the Deployment utils things yet, since I fear those are the more interesting parts to review, so being able to view them somewhat in isolation is a good thing (hint, don't squash it in the current changes :))

@floreks floreks force-pushed the client-go-migration branch 2 times, most recently from efe6349 to f4da751 Compare February 15, 2017 13:53
@codecov-io
Copy link

codecov-io commented Feb 15, 2017

Codecov Report

Merging #1638 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1638   +/-   ##
=======================================
  Coverage   93.21%   93.21%           
=======================================
  Files         372      372           
  Lines        3139     3139           
=======================================
  Hits         2926     2926           
  Misses        213      213

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3a0bb23...f4052c7. Read the comment docs.

@floreks
Copy link
Member Author

floreks commented Feb 16, 2017

@rf232 @ianlewis @maciaszczykm @mijajasinski

Most of the work is done. Travis is green. Only thing left is to fix some tests but I need to look a bit deeper into some of them because of the migration and this may take some time. Could you test it and maybe merge? Faster we merge, easier it will be for others to rebase their changes.

I can create issue to fix skipped tests and work on that separately.

@rf232
Copy link
Contributor

rf232 commented Feb 16, 2017

Most of the work is done. Travis is green. Only thing left is to fix some tests but I need to look a bit deeper into some of them because of the migration and this may take some time. Could you test it and maybe merge? Faster we merge, easier it will be for others to rebase their changes.

Agreed, reviewing now

I can create issue to fix skipped tests and work on that separately.

SGTM

Copy link
Contributor

@rf232 rf232 left a comment

Choose a reason for hiding this comment

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

To get the diamond zombie slayer achievement please run:
govendor list +u | cut -c5- | xargs govendor remove

@@ -0,0 +1,100 @@
package util
Copy link
Contributor

Choose a reason for hiding this comment

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

It is a shame we need to copy this logic from the kubernetes main repo. I think it is indeed the best option at the moment. It might be worth it to ask client-go if they can get this functionality too, so we can lift it from them and trust on them to keep the logic in sync.

Copy link
Member Author

@floreks floreks Feb 16, 2017

Choose a reason for hiding this comment

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

Ye, It would be great if they could move utils also. Keeping deployment utils deps also downloads significant amount of files so I guess it's better for now to just copy these few functions and keep them in sync.

I've just checked and we do have them in our kubernetes deps. So maybe I will reuse them anyway since I have already updated kubernetes deps also and it is same version that should not conflict with client-go.

Copy link
Member Author

Choose a reason for hiding this comment

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

Just tried to reuse them and unfortunately the are using different extensions package version. We have to leave it as is for now.

@floreks
Copy link
Member Author

floreks commented Feb 16, 2017

To get the diamond zombie slayer achievement please run:
govendor list +u | cut -c5- | xargs govendor remove

Good tip. Achievement unlocked 😃

@maciaszczykm maciaszczykm merged commit 4c366bc into kubernetes:master Feb 16, 2017
@floreks floreks deleted the client-go-migration branch February 16, 2017 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants