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

regenerate client code with latest k8s api #682

Merged

Conversation

eastlondoner
Copy link

@eastlondoner eastlondoner commented Nov 5, 2018

So that it is compatible with python 3.7

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 5, 2018
@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Nov 5, 2018
@roycaihw
Copy link
Member

roycaihw commented Nov 7, 2018

Please see #584 (comment).

The python 3.7 support issue was about the reserved keyword workaround. It's not related to Kubernetes API (openapi spec). We already have two GA releases that support Python 3.7 (v7.0.0 and v8.0.0). Please try the new versions out.

I don't see strong reason for re-generating v6 client against Kubernetes release-1.10 spec. Closing

/close

@k8s-ci-robot
Copy link
Contributor

@roycaihw: Closing this PR.

In response to this:

Please see #584 (comment).

The python 3.7 support issue was about the reserved keyword workaround. It's not related to Kubernetes API (openapi spec). We already have two GA releases that support Python 3.7 (v7.0.0 and v8.0.0). Please try the new versions out.

I don't see strong reason for re-generating v6 client against Kubernetes release-1.10 spec. Closing

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@roycaihw roycaihw reopened this Nov 7, 2018
@roycaihw
Copy link
Member

roycaihw commented Nov 7, 2018

More thoughts on this:

Kubernetes supports three minor releases at the same time. Currently it's 1.10, 1.11 and 1.12. This python client repo follows similar policy and we currently support v6, v7 and v8. Kubernetes v1.13.0 is planned to be released in less a month (Dec. 3rd). After k8s 1.13 is released, 1.10 will not be supported, and we will start releasing python client v9 and dropping v6.

At this point, given the short period of supporting 1.10, I'd suggest switching to more recent versions of Kubernetes, instead of releasing a python client v6.0.1 to support Python 3.7

[update] Also please take a look at #681 (comment). Please try 7.0.0 release to see if it works better with k8s 1.10 apiserver

@eastlondoner
Copy link
Author

eastlondoner commented Nov 7, 2018

@roycaihw

The python 3.7 support issue was about the reserved keyword workaround.

Yes. In order to get the fix for the reserved keyword it is necessary to do what I have done in this PR - do you disagree?

We already have two GA releases that support Python 3.7 (v7.0.0 and v8.0.0).

I would like to use python 3.7 with v6 because I want to use K8 1.10 which is supposed to be supported now.

At this point, given the short period of supporting 1.10, I'd suggest switching to more recent versions of Kubernetes, instead of releasing a python client v6.0.1 to support Python 3.7

I suggest that you merge my PR and support all the versions of K8s that are supposed to be supported.

That sounds good in principle. We are using hosted Kubernetes with GCP and 1.11 has only been available for general use for less than a week, we need some time to ensure that we can move to 1.11 without problems.

Do you have an actual reason not to merge this PR?

@k8s-ci-robot k8s-ci-robot requested a review from yliaog November 7, 2018 22:46
Copy link
Member

@roycaihw roycaihw left a comment

Choose a reason for hiding this comment

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

@eastlondoner Fair enough. This covers a gap in our support for Kubernetes 1.10 with Python 3.7.

Please add a section to the CHANGELOG noting what's changed in v6.1.0 (actually bumping minor version number is not enough, as we are introducing breaking change here):

Once we merge this PR we can start making Pypi distribution packages and Github release page.

/cc @yliaog

```
(you may need to run `pip` with root permission: `sudo pip install git+https://github.com/kubernetes-client/python.git`)
(you may need to run `pip` with root permission: `sudo pip install git+https://github.com/kubernetes-kubernetes.client/python.git`)
Copy link
Member

Choose a reason for hiding this comment

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

Please manually restore the commands in L20 and L22. I believe we have some misconfig of automation/swagger-codegen that writes the extra kubernetes.

@@ -79877,7 +79877,7 @@
"type": "string"
},
"mountPropagation": {
"description": "mountPropagation determines how mounts are propagated from the host to container and the other way around. When not set, MountPropagationHostToContainer is used. This field is beta in 1.10.",
"description": "mountPropagation determines how mounts are propagated from the host to container and the other way around. When not set, MountPropagationNone is used. This field is beta in 1.10.",
Copy link
Member

Choose a reason for hiding this comment

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

Looks like there is no significant api change from openapi spec :)

@eastlondoner
Copy link
Author

Thanks @roycaihw.
I have updated the README, CHANGELOG, setup.py and tox.ini

tox.ini Outdated
commands =
python -V
{toxinidir}/scripts/kube-init.sh py.test -vvv -s []

Copy link
Contributor

Choose a reason for hiding this comment

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

I’m not sure we test Python 3.7 functional in our Travis CI even in the master branch. Also, I’m not sure if official support for Travis CI has been extended to Python 3.7. See the most recent Travis build.

Need confirmation from @roycaihw

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

ah, well I guess this is future proof at least

Copy link
Member

Choose a reason for hiding this comment

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

We haven't supported Python 3.7 in our travis-ci yet. Please see #584

Changes (tox.ini and .travis.yml) need to be checked into master branch in both python and python-base. I'm more comfortable with having separate PRs for those changes and this PR including necessary client re-generation. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Me too. I prefer to wait to see Travis CI support Python 3.7 and make sure the keywords are correct, and then check them into the master branch.

@roycaihw
Copy link
Member

roycaihw commented Nov 8, 2018

I realized that I missed mentioning the step to update version constant. Please regenerate the client with the following steps:

  1. update CLIENT_VERSION in scripts/constants.py to be 6.1.0 (ref: https://github.com/kubernetes-client/python/blob/master/devel/release.md#update-release-tags)
  2. run scripts/update-client.sh and remove the extra "kubernetes." in README (swagger-codegen will update the client versions for you, so you don't have to manually change them)
  3. add release note to CHANGELOG

@eastlondoner
Copy link
Author

Done.
I reverted and started over. Changes are in a single commit. No changes to tox.ini

@roycaihw
Copy link
Member

roycaihw commented Nov 8, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 8, 2018
@yliaog
Copy link
Contributor

yliaog commented Nov 8, 2018

/lgtm

@roycaihw
Copy link
Member

roycaihw commented Nov 8, 2018

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: eastlondoner, roycaihw
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: caesarxuchao

If they are not already assigned, you can assign the PR to them by writing /assign @caesarxuchao in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@roycaihw roycaihw merged commit 6bcca10 into kubernetes-client:release-6.0 Nov 8, 2018
@eastlondoner
Copy link
Author

Thanks :-D this helps us to upgrade our stack smoothly as we work towards 1.11

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. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants