Fix: Make dependancy adal optional

[iamneha]

No description provided.

[micw523]

This will cause adal to be installed automatically for Python 2.7.* , but not Python 3.* with setuptools. Since python-base does not have its own requirements.txt, it may cause kube_configto fail.

[iamneha]

@micw523 can you tell me how to test this locally.

[micw523]

I am afraid this won’t work - see Travis CI information. Supported environment markers are documented in PEP 496 (
https://www.python.org/dev/peps/pep-0496/).

To test your changes, you’ll need to have a Python 2.7 and a python 3.4, 3.5, 3.6 environment (as specified in Travis CI, but usually one of 3.4/5/6 is enough). Use setuptools to install your package from your local source, and use python -m unittest discover to test your code. Changing adal to be optional will at least require restructuring of existing code to let it be allowed as an extra install component and will break existing user experience using Azure. I suggest you consult the owners of the repo on how they want to do it.

[iamneha]

@micw523 I'm testing the same way you said.
Adding this in requirements.txt

adal>=1.0.2;python_version=='2.7' # MIT
adal>=1.0.2;python_version=='3.5' # MIT
adal>=1.0.2;python_version=='3.6' # MIT
adal>=1.0.2;python_version=='3.7' # MIT

All the test are passing locally but I am not sure if this is a good way to do.
Let me talk to the maintainer of this project.

[micw523]

You probably have a local copy of adal. You can try using pip to list your installed packages. Try start with a fresh python environment and install from your local source with setuptools.

I think you probably has misunderstood me - I was simply saying python_version='3' is not a valid environment marker. You'll need to restructure the code in kubernetes/python-base. Most likely there will be only a one-line change in the requirements.txt.

[roycaihw]

I'd expect marking the dependency as optional would require some breaking change, since users depending on the feature have to opt-in (or we could default on, and allow users to opt-out to be maximum backward compatible-- new optional dependency should be defaulted off)

At any rate, we should document the change well

[tomplus]

https://setuptools.readthedocs.io/en/latest/setuptools.html#declaring-extras-optional-features-with-their-own-dependencies

It was suggested in the issue as a twitter comment and it looks like what we need.

[micw523]

@tomplus Yes that’s where it is and our requirements are stored in the requirements.txt and are being broken up into regular requirements and extra requirements. We are currently specifying setuptools >= 0.20 so we could not use env markers directly in the regular installs (introduced in 0.36.0)

Since the adal is used in python-base/config, it’s highly likely that we’ll need to segment it out and define a new entry point for azure users, otherwise it would probably break the CI. I haven’t tested it though since Neha is on this issue.

[iamneha]

@micw523 you are right. I went through the code whole day and seems like changing requirement is not going to solve this issue.

[tomplus]

I thought about simpler solution:

1. remove adal from requirements
2. add extras_require to setup.py if setuptools>=0.36
3. handle import error or use lazy loading

try:
   import adal
except ImportError:
   pass
...
...
def _refresh_azure_token():
    if 'adal' not in globals():
        raise Error('refresh token error, adal library not imported')

4. update doc/readme that adal have to be installed manually if you are integrated with MS Active Directory

[micw523]

@tomplus That sounds good to me. We don’t need setuptools version 0.36 or up - it’s only that we can put everything in the requirements.txt into the installs_require block and simplify the setup.py logic. If we’re using adal for extras require I think 0.20 should be fine.

It’s relatively simple for user to install adal by themselves, see the end of the page at https://packaging.python.org/tutorials/installing-packages/#installing-setuptools-extras

[iamneha]

@tomplus can you look into this as well. The test is failing after I added adal into the extra_required
The possible fix for this is to handle import error in the test as well.

[tomplus]

@iamneha There are not tests dedicated for the adal integration. These tests will pass if your previous PR (kubernetes-client/python-base#108) is merged.

[iamneha]

@tomplus that make sense to me. Thanks for the help :)

[k8s-ci-robot]

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

---

• If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
• If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
• If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: [email protected]

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. I understand the commands that are listed here.

[roycaihw]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: iamneha, 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:

• OWNERS [roycaihw]

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

[ktdreyer]

Thank you for merging this!

Would you please release a new version of this library to PyPI with the adal requirement removed? It would be great to have this fixed in an official version.