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

Add support for thirdparty resource watch/create/put and delete operations #216

Merged
merged 13 commits into from
May 18, 2017

Conversation

jonathan-kosgei
Copy link
Contributor

@jonathan-kosgei jonathan-kosgei commented May 9, 2017

Fixes #201

Support third-party resources

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 9, 2017
@codecov-io
Copy link

codecov-io commented May 9, 2017

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #216   +/-   ##
=======================================
  Coverage   94.56%   94.56%           
=======================================
  Files           9        9           
  Lines         681      681           
=======================================
  Hits          644      644           
  Misses         37       37

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 7a6bae2...e469509. Read the comment docs.

@jonathan-kosgei
Copy link
Contributor Author

How do I name the classes in swagger? So that we don't get something like DefaultAPi as the api_instance object?

@mbohlool
Copy link
Contributor

mbohlool commented May 9, 2017

@jonathan-kosgei add a tag to operations. Look at our swagger.json for examples.

with open('thirdpartypaths.json', 'r') as third_party_spec_file:
third_party_spec = json.loads(third_party_spec_file.read())
for path in third_party_spec.keys():
spec['paths'][path] = third_party_spec[path]
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you just add a validation here to make sure we are not overwriting an existing path?

@mbohlool
Copy link
Contributor

mbohlool commented May 9, 2017

one nit comment. Let me know when you added the tags for better naming and one example. This looks good so far.

(btw, use git commit --amend for small commits that should be part of the previous commit. you can merge that one small commit by git rebase -i HEAD~2)

@jonathan-kosgei
Copy link
Contributor Author

Awesome tip. Thanks. Pushing changes, including a example. I'll add a few more as I go along.

from kube_resource.rest import ApiException
from pprint import pprint

with open('/var/run/secrets/kubernetes.io/serviceaccount/token', 'r') as token_file:
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use load_kube_config like other examples.

resource = 'repos'
fqdn = 'git.k8s.com'

body = json.loads('{ "apiVersion": "git.k8s.com/v1", "kind": "RePo", "metadata": { "name": "blog-repo" }, "repo": "github.com/user/my-blog", "oauth": 123456789, "branch": "master", "key": "user-git-deploy-secret", "path": "/path/in/volume", "image": "image to run job in", "then": "hugo --destination=/home/user/hugosite/public", "pvc": "persistent-volume-claim", "username": "repo username", "password": "repo password"}')
Copy link
Contributor

Choose a reason for hiding this comment

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

I would just make it a python map. The syntax is almost identical to json.

kube_resource.configuration.api_key['Authorization'] = token
kube_resource.configuration.api_key_prefix['Authorization'] = 'Bearer'
kube_resource.configuration.ssl_ca_cert = '/var/run/secrets/kubernetes.io/serviceaccount/ca.crt'
api_instance = kube_resource.DefaultApi()
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought you renamed DefaultAPi using tags?

"summary": "Gets Resources",
"description": "Returns a list of Resources",
"tags": [
"thirdparty_v1"
Copy link
Contributor

Choose a reason for hiding this comment

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

Name Suggestion : third_party_resource?

Copy link
Contributor

Choose a reason for hiding this comment

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

third_party_resources

@mbohlool
Copy link
Contributor

Look like there are some formatting error too. Use script/update-pep8.sh

@jonathan-kosgei
Copy link
Contributor Author

On it.

@pvdvreede
Copy link

This is something I would really like so that I can use the python client to create a custom operator. Will this be merged/released soon?

Is there a current way to watch a specific third party resource I create in version 2.0 as a stop gap?

@mbohlool
Copy link
Contributor

@jonathan-kosgei any update on this? Can I help with anything?
I too hope this merge in soon.

@mbohlool
Copy link
Contributor

The code looks good. I will Squash and merge after tests passed.

@jonathan-kosgei
Copy link
Contributor Author

Hi @pvdvreede Sorry, got caught up with work, fixed

@pvdvreede
Copy link

Great! Thanks very much! Hopefully a new release can be done soon?

@mbohlool
Copy link
Contributor

@jonathan-kosgei The client needs to be updated with the new yaml changes. I will send a follow up PR tomorrow for that.
@pvdvreede We need to finalize the interface and add some more tests before we cut a release. Would be nice if you can install the client from the master and give this a try and maybe add some examples so we know the interface is good enough. I will do some work on this soon.

@mbohlool mbohlool merged commit dab09e0 into kubernetes-client:master May 18, 2017
@jonathan-kosgei jonathan-kosgei deleted the dev branch May 18, 2017 08:00
@pvdvreede
Copy link

Thanks @mbohlool I gave this a go last night and used the master branch. It errored saying there was no ThirdPartyResources attribute on kubernetes module. So I am not sure how to get the api instance. Does the example need updating/changing?

@jonathan-kosgei
Copy link
Contributor Author

jonathan-kosgei commented May 18, 2017

I think probably the client needs to be regenerated? @mbohlool
Though I think that would depend on the release planning already in place?
You can clone the code locally and run the update-client script in the scripts folder. In the meantime take a look at https://github.com/jonathan-kosgei/kubeResource, I've tested that, and used it as the base for this pr

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.

None yet

5 participants