-
Notifications
You must be signed in to change notification settings - Fork 48
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
Chore/python3 #646
Chore/python3 #646
Conversation
The style in this PR agrees with This formatting comment was generated automatically by a script in uc-cdis/wool. |
Pull Request Test Coverage Report for Build 7256
💛 - Coveralls |
9c28af5
to
8d4be60
Compare
cde1d09
to
941f912
Compare
Dockerfile
Outdated
|
||
COPY . /fence | ||
# number of uwsgi worker processes | ||
ENV UWSGI_CHEAPER 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is already set in the base image https://github.com/uc-cdis/cloud-automation/blob/master/Docker/python-nginx/python3.6-alpine3.7/Dockerfile#L168-L169
fence/jwt/keys.py
Outdated
@@ -181,8 +181,13 @@ def public_key_to_jwk(self): | |||
Return: | |||
dict: JWK representation of the public key | |||
""" | |||
n, e = _rsa_public_numbers(self.public_key) | |||
# n, e = _rsa_public_numbers(self.public_key) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can fully remove this
tests/test_import_fence.py
Outdated
@@ -22,15 +22,15 @@ def reload_modules(module_name): | |||
# actually exist, even if it does. To do this, patch-delete the attribute | |||
# and reload all the fence modules. | |||
fence_submodules = [ | |||
module for module in sys.modules.keys() if module.startswith(module_name) | |||
module for module in list(sys.modules.keys()) if module.startswith(module_name) | |||
] | |||
for module in fence_submodules: | |||
if sys.modules[module]: | |||
# SQLAlchemy gets upset when a table is loaded twice, so ignore | |||
# that. | |||
try: | |||
# NOTE: in python3 this should become ``importlib.reload`` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# NOTE: in python3 this should become ``importlib.reload`` |
tests/test_logout.py
Outdated
import urlparse | ||
import urllib | ||
import urllib.request, urllib.parse, urllib.error | ||
from fence.resources.storage.cdis_jwt import create_session_token |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this needed? it doesn't look like it was there before
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's duplicated :o looks like the imports have been messed up during a rebase to master, thanks will fix
@@ -101,6 +101,7 @@ At the moment, supported IDPs include: | |||
- InCommon | |||
- eduGAIN | |||
|
|||
Note: the Shibboleth dockerfile image is at https://quay.io/repository/cdis/fence-shib and is NOT compatible with python 3/the latest fence. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a specific reason for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i didn't update the shibboleth dockerfile to work with python 3 code for now, I discussed with Rudy and he said master fence probably won't be deployed to the commons that's using this dockerfile
message
attribute anymoreenum34
dependency because it is not compatible with the standard library'senum
on python >3.4test_update_user_service_account_success
TODO:
pin versions after PRs are merged:
Breaking Changes
Dependency updates