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

Generate 11.0.0-snapshot client #995

Merged
merged 8 commits into from
Nov 15, 2019

Conversation

roycaihw
Copy link
Member

@roycaihw roycaihw commented Oct 29, 2019

Updated python-base submodule, collected CHANGELOG from python-base and 11.0.0a1, and generated 11.0.0-snapshot client

Added an alias package with deprecation message for the major breaking change as #974 (comment) suggested
/cc @oz123

TODO:

  • wait for test to be green (our test code in master branch still imports the deprecated package)
  • collect CHANGELOG for API changes since 11.0.0a1
  • install locally and test the deprecation warning behavior

@k8s-ci-robot k8s-ci-robot requested a review from oz123 October 29, 2019 16:50
@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Oct 29, 2019
@roycaihw
Copy link
Member Author

diff between 11.0.0-snapshot and 11.0.0a1:

$ git diff 11.0.0-snapshot..upstream/release-11.0 --stat | grep -v "|   2 +-" | grep -v "examples\/" | grep -v "e2e_test\/"
 .travis.yml                                        |   4 -
 CHANGELOG.md                                       |  18 ++-
 README.md                                          |  36 ++++--
 RELEASE.md                                         |   8 +-
 kubernetes/README.md                               |   4 +-
 kubernetes/client/__init__.py                      |   4 +-
 kubernetes/client/api/custom_objects_api.py        |  34 ++----
 kubernetes/client/api_client.py                    |   4 +-
 kubernetes/client/apis/__init__.py                 |  11 --
 kubernetes/client/configuration.py                 |   6 +-
 kubernetes/docs/CustomObjectsApi.md                |  28 ++---
 kubernetes/utils/__init__.py                       |   1 -
 kubernetes/utils/quantity.py                       |  75 ------------
 scripts/constants.py                               |   4 +-
 scripts/rest_client_patch.diff                     |   4 +-
 scripts/swagger.json                               |  36 +-----
 scripts/update-client.sh                           |   7 ++
 setup.py                                           |   7 +-
 test-requirements.txt                              |   1 -
 tox.ini                                            |  29 ++++-

@roycaihw roycaihw added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 29, 2019
Copy link
Contributor

@oz123 oz123 left a comment

Choose a reason for hiding this comment

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

@roycaihw nicely done.
I thought it would be more work ...
You forgot to update the examples.
examples/pod_exec.py has one import line that needs to be updated. Can you please update this?

The warning work fine though, here is how this example output looks like when running Python with warnings enabled:

 $ python -W all  examples/pod_exec.py 
/home/oznn/Software/k8s-python/kubernetes/client/apis/__init__.py:10: DeprecationWarning: The package kubernetes.client.apis is renamed and deprecated, use kubernetes.client.api instead (please note that the trailing s was removed).
  DeprecationWarning
Response: This message goes to stderr
This message goes to stdout

Running command... echo This message goes to stdout

STDOUT: This message goes to stdout

Running command... echo "This message goes to stderr" >&2

STDERR: This message goes to stderr

Server date command returns: Tue Oct 29 20:53:47 UTC 2019
Server user is: root
sys:1: ResourceWarning: unclosed <ssl.SSLSocket fd=7, family=AddressFamily.AF_INET, type=SocketKind.SOCK_STREAM, proto=6, laddr=('192.168.1.108', 60596), raddr=('213.95.155.51', 6443)>
sys:1: ResourceWarning: unclosed <ssl.SSLSocket fd=6, family=AddressFamily.AF_INET, type=SocketKind.SOCK_STREAM, proto=6, laddr=('192.168.1.108', 60594), raddr=('213.95.155.51', 6443)>

Notice: I am actively enabling a flag in Python. Which means not all the users are going to see these warnings. Let's hope everyone has good tests :-) Pytest runs all the tests with warnings enable by default.

@roycaihw
Copy link
Member Author

roycaihw commented Nov 1, 2019

@oz123 Ah I didn't enable the flag so I didn't see the warning message when I tested. Thanks for verifying it! I'm curious if the log level is configurable to make the warning visible all the time.

@roycaihw roycaihw changed the title [WIP] Generate 11.0.0-snapshot client Generate 11.0.0-snapshot client Nov 11, 2019
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 11, 2019
@roycaihw
Copy link
Member Author

Okay with 950df11 the deprecation warning is printed by default when the old module gets imported. @oz123 WDYT?

The warning filters are defined here.

@oz123
Copy link
Contributor

oz123 commented Nov 11, 2019

@roycaihw I think it's a good idea to enable the warning per default.

@oz123
Copy link
Contributor

oz123 commented Nov 11, 2019

/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 11, 2019
@roycaihw
Copy link
Member Author

@oz123 Thanks for the review!

/assign @yliaog

@drolando
Copy link

If I understand the docs correctly I think 950df11 is turning warnings on for everything. That means that if my code uses both the kubernetes client and other libraries, you're turning on the warnings also for those entirely unrelated libraries.
I didn't try this, but I think you should specify also module and make that apply only to the kubernetes module

@roycaihw
Copy link
Member Author

@drolando You're right. It turns on warning for unrelated paths. E.g.

import kubernetes.client.apis
import warnings
warnings.warn(
    "another warning.",
    DeprecationWarning
)
.../kubernetes/client/apis/__init__.py:13: DeprecationWarning: The package kubernetes.client.apis is renamed and deprecated, use kubernetes.client.api instead (please note that the trailing s was removed).
  DeprecationWarning
example.py:5: DeprecationWarning: another warning.
  DeprecationWarning

I didn't try this, but I think you should specify also module and make that apply only to the kubernetes module

could you explain a bit more?

@drolando
Copy link

I was looking at https://docs.python.org/3/library/warnings.html#the-warnings-filter and it says you can pass in a module argument to limit the scope.

So you'd probably want something like warnings.filterwarnings('default', module='kubernetes')

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 12, 2019
@roycaihw
Copy link
Member Author

@drolando Thanks for the suggestion. I verified that the example worked with b316493.

@roycaihw
Copy link
Member Author

/hold cancel
/cc @yliaog
This pull is ready for review.

@k8s-ci-robot k8s-ci-robot requested a review from yliaog November 12, 2019 20:54
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 12, 2019
@roycaihw
Copy link
Member Author

@yliaog gentle ping

@@ -1,3 +1,43 @@
# v11.0.0b1
Copy link
Contributor

Choose a reason for hiding this comment

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

so you're releaseing a1 and b1 together?

Copy link
Member Author

Choose a reason for hiding this comment

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

no. This is copying the a1 release note from the release branch, since we didn't updated the release note in the master branch when we did a1 release.

@yliaog
Copy link
Contributor

yliaog commented Nov 15, 2019

/lgtm
/approve

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: roycaihw, yliaog

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. 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