Skip to content

Conversation

@uvdn7
Copy link
Contributor

@uvdn7 uvdn7 commented Sep 11, 2015

No description provided.

@boxcla
Copy link

boxcla commented Sep 11, 2015

Verified that @aptxkid has signed the CLA. Thanks for the pull request!

@jmoldow
Copy link
Contributor

jmoldow commented Sep 16, 2015

What is this? I don't see anything about this at https://developers.box.com/developer-edition/.

It sucks to not be able to test with the latest and greatest cryptography. It looks like Travis CI has an old version of PyPy. They have 2.5, but PyPy is up to 2.6. See travis-ci/travis-ci#4756.

The cryptography project itself IS able to test >1.0 on the latest PyPy. They install pyenv in order to be able to pull in arbitrary versions of Python, and they use that to install the latest version of PyPy. https://github.com/pyca/cryptography/blob/master/.travis/install.sh#L57

This doesn't seem to negatively affect their ability to run with sudo: false. https://github.com/pyca/cryptography/blob/master/.travis.yml#L1

I might experiment with doing that, since I think that'd be better than testing with a pre-1.0 versions of cryptography. What do you think?

@jmoldow
Copy link
Contributor

jmoldow commented Sep 16, 2015

Okay, I fixed the PyPy issue in master. Can you rebase or merge on master, and remove your PyPy / cryptography-related changes?

@uvdn7
Copy link
Contributor Author

uvdn7 commented Sep 16, 2015

Done. Thanks, @jmoldow !!

requirements.txt Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

These need to be removed, they break the build on Python 3.5. I removed them in the commit I merged to master.

Copy link
Contributor

Choose a reason for hiding this comment

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

In case you're wondering, we still install them, but it happens in setup.py, protected by an if-statement for Python 2.

The dot at the bottom of this file makes sure that we install boxsdk itself, and also that we run setup.py. That will pick up enum34 and ordereddict when necessary.

BTW, can you change the .at the bottom of this file to -e .? Thanks.

@jmoldow
Copy link
Contributor

jmoldow commented Sep 17, 2015

Remove "I also restricted the version of cryptography to be <1.0. Otherwise it won't work for pypy." from your commit message.

Copy link
Contributor

Choose a reason for hiding this comment

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

To be consistent, can you:

  • change id to ID
  • change jwt to JWT
  • end with a period

@jmoldow
Copy link
Contributor

jmoldow commented Sep 17, 2015

👍 but please address comments.

@landscape-bot
Copy link

Code Health
Code quality remained the same when pulling fe2c5a5 on box:jwt-key-rotation into 63b60a2 on box:master.

@jmoldow
Copy link
Contributor

jmoldow commented Sep 17, 2015

This looks good now (though if you get a chance, maybe remove the part of your commit message that is no longer accurate).

Since this is a new required parameter, I think we should hold off on merging this until the public documentation is made available at https://box-content.readme.io/v2.0/docs/constructing-the-json-web-token-jwt-assertion.

This is a breaking change. But since Box Developer Edition is still in beta, we can probably do just a minor version bump.

@uvdn7
Copy link
Contributor Author

uvdn7 commented Sep 17, 2015

@jmoldow Sure. I wasn't planning merging it either. :)

@landscape-bot
Copy link

Code Health
Code quality remained the same when pulling da79dbe on box:jwt-key-rotation into 63b60a2 on box:master.

Jeff-Meadows added a commit that referenced this pull request Jan 5, 2016
Add 'kid' (jwt key id) to JWTAuth header
@Jeff-Meadows Jeff-Meadows merged commit 2d2a585 into master Jan 5, 2016
@Jeff-Meadows Jeff-Meadows deleted the jwt-key-rotation branch January 5, 2016 19:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants