Skip to content
This repository has been archived by the owner on Mar 13, 2022. It is now read-only.

Make dependancy adal optional #108

Merged
merged 1 commit into from
Jan 23, 2019

Conversation

iamneha
Copy link
Contributor

@iamneha iamneha commented Dec 11, 2018

Fixes issue :kubernetes-client/python/issues#645
To fix this issue:

  1. remove adal from requirements
  2. add adal to extras_require in setup.py
  3. handle import error

Fix for 1 and 2 is kubernetes-client/python/pull#668

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 11, 2018
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Dec 11, 2018
@iamneha
Copy link
Contributor Author

iamneha commented Dec 11, 2018

/cc @tomplus @roycaihw @dims

@k8s-ci-robot k8s-ci-robot requested review from dims and tomplus December 11, 2018 17:23
config/kube_config.py Outdated Show resolved Hide resolved
@codecov-io
Copy link

Codecov Report

Merging #108 into master will decrease coverage by 0.3%.
The diff coverage is 11.76%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #108      +/-   ##
==========================================
- Coverage   92.04%   91.74%   -0.31%     
==========================================
  Files          13       13              
  Lines        1182     1187       +5     
==========================================
+ Hits         1088     1089       +1     
- Misses         94       98       +4
Impacted Files Coverage Δ
config/kube_config.py 83.22% <11.76%> (-1.02%) ⬇️

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 5c242ea...cfae0d6. Read the comment docs.

@tomplus
Copy link
Member

tomplus commented Dec 11, 2018

BTW, there is a similar PR: #101

@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 11, 2018
@iamneha
Copy link
Contributor Author

iamneha commented Dec 11, 2018

BTW, there is a similar PR: #101

I don't know how this happened I always assign myself to issue I am working on.
I think @micw523 started this after I raised my first PR.

@micw523
Copy link
Contributor

micw523 commented Dec 12, 2018

Per our slack discussion on Oct 31st, I asked you if you wanted to hand over the issue and you said yes. That was why I started to work on it and opened a PR.

If you want to work on it again, go straight ahead. I don’t mind - to me reviewing is just important as submitting a fix. However in the future please remember what you have said so that this situation does not happen any more.

@iamneha
Copy link
Contributor Author

iamneha commented Dec 13, 2018

Per our slack discussion on Oct 31st, I asked you if you wanted to hand over the issue and you said yes. That was why I started to work on it and opened a PR.

Yes @micw523 you asked me the issue you were working on but I think you are confusing this issue with another one you asked for
This is the issue you asked for " Loading yaml file "

If you want to work on it again, go straight ahead. I don’t mind - to me reviewing is just important as submitting a fix. However in the future please remember what you have said so that this situation does not happen any more.

I think we should start working on issue after assigning ourselves. I would have stopped working on it if you mentioned that you are working on it.

No worries, if you still want to work on it please assign yourself to that issue and I'm happy to help you with the issue :)

@dims
Copy link

dims commented Dec 16, 2018

@iamneha @micw523 i left a note in #101

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.

/lgtm

@iamneha @micw523 Thanks for working on this :)

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 23, 2019
@roycaihw roycaihw added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 23, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by: iamneha

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 merged commit 260f855 into kubernetes-client:master Jan 23, 2019
drewwalters96 pushed a commit to drewwalters96/armada that referenced this pull request Apr 25, 2019
This version [0] makes the adal package optional [1], which also
removes its dependency on the cryptography package.

It also fixes a thread pool issue [2] allowing us to remove the
workaround we had in place.

[0]: https://github.com/kubernetes-client/python/blob/master/CHANGELOG.md#v900
[1]: kubernetes-client/python-base#108
[2]: kubernetes-client/gen#91

Change-Id: I55aa8b97483b118fbde7e11df817ad8330da9bf1
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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 Indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants