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

Update OpenAPI generator and the client #1188

Merged

Conversation

palnabarun
Copy link
Member

@palnabarun palnabarun commented Jun 22, 2020

This PR also:

  1. Remove tests since they are stubs
  2. Updates the CHANGELOG with changes from 1.16.9 to 1.16.11
  3. Updates the CHANGELOG with changes from python-base
  4. Update the hotfix script

Follow ups:

  • Update the release documentation

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

/assign @roycaihw @yliaog

@palnabarun
Copy link
Member Author

There is a test failure in the base submodule:

        self.assertEqual(expected_token,
>                        config.auth_settings()['BearerToken']['value'])
E       KeyError: 'BearerToken'

kubernetes/base/config/kube_config_test.py:1373: KeyError

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.

The commits LGTM in general. Once we collect the API change caused by openapi-generator in CHANGELOG we will be good to go.

@@ -1,39 +0,0 @@
# coding: utf-8
Copy link
Member

Choose a reason for hiding this comment

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

@roycaihw
Copy link
Member

There is a test failure in the base submodule

I checked python-base and found there was one pycodestyle failure merged accidentally. I sent a fix: kubernetes-client/python-base#200.

The error in #1188 (comment) looks different though. I created an experimental PR #1191 that only bumps python-base, to help us better identify if the bug is on our side or introduced by openapi-generator.

@roycaihw
Copy link
Member

The tests passed in #1191. Looks like the KeyError is related to openapi-generator change.

@ACXLM ACXLM mentioned this pull request Jun 26, 2020
@roycaihw
Copy link
Member

The test failure

    def test_auth_settings_calls_get_api_key_with_prefix(self):
        expected_token = 'expected_token'
    
        def fake_get_api_key_with_prefix(identifier):
            self.assertEqual('authorization', identifier)
            return expected_token
        config = Configuration()
        config.get_api_key_with_prefix = fake_get_api_key_with_prefix
        self.assertEqual(expected_token,
>                        config.auth_settings()['BearerToken']['value'])
E       KeyError: 'BearerToken'

is related to the upstream change OpenAPITools/openapi-generator#4988. In the test we faked a Configuration with a customized get_api_key_with_prefix, but we didn't set the api_key thus the returned attribute was empty. We should append authorization to api_key of the fake Configuration in the test.

@palnabarun
Copy link
Member Author

/reopen

(the bot mistakenly closed this due to kubernetes-client/python-base#202 merging)

@k8s-ci-robot
Copy link
Contributor

@palnabarun: Reopened this PR.

In response to this:

/reopen

(the bot mistakenly closed this due to kubernetes-client/python-base#202 merging)

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.

@k8s-ci-robot k8s-ci-robot reopened this Jul 16, 2020
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 16, 2020
The upstream 1.16 API moved ahead due to [patch releases](https://github.com/kubernetes/sig-release/blob/master/releases/patch-releases.md).

The logs are gathered from the upstream [CHANGELOG](https://raw.githubusercontent.com/kubernetes/kubernetes/master/CHANGELOG/CHANGELOG-1.16.md).

Signed-off-by: Nabarun Pal <[email protected]>
The implementation and tests were already picked up by the upstream OpenAPI
generator [here](OpenAPITools/openapi-generator#5094).
Patching in the tests here for correctness and clarity.

Reference: kubernetes-client#1073

Signed-off-by: Nabarun Pal <[email protected]>
Signed-off-by: Nabarun Pal <[email protected]>
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 16, 2020
@palnabarun
Copy link
Member Author

The build is again failing due to a regression caused in kubernetes-client/python-base#133.

Ref: https://travis-ci.org/github/kubernetes-client/python-base/builds/708448505

@palnabarun
Copy link
Member Author

kubernetes-client/python-base#204 will fix the test failure.

Update from 49ec060 to fb86b8a

Signed-off-by: Nabarun Pal <[email protected]>
@palnabarun
Copy link
Member Author

After kubernetes-client/python-base#204 got merged, the tests are passing.

CHANGELOG.md Outdated
- Allow incluster to accept pass-in config [kubernetes-client/python-base#193](https://github.com/kubernetes-client/python-base/pull/193)
- Set expiration on token of incluster config and reload the token if it expires [kubernetes-client/python-base#191](https://github.com/kubernetes-client/python-base/pull/191)

**Bug Fix:**

- Fixes a bug in loading kubeconfig when there are no users in ttthe config [kubernetes-client/python-base#198](https://github.com/kubernetes-client/python-base/pull/198)
Copy link
Contributor

Choose a reason for hiding this comment

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

typo ttthe

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the catch. 👍 It is resolved now.

The changes have been listed in the CHANGELOG and linked to the
respective milestone pull requests.

Signed-off-by: Nabarun Pal <[email protected]>
@roycaihw
Copy link
Member

/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 Jul 20, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 20, 2020
@k8s-ci-robot k8s-ci-robot merged commit bc79976 into kubernetes-client:master Jul 20, 2020
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.

4 participants