Conversation
Upgrade HTTPretty to version that supports Python3.7 Upgrade Recurly to version that supports Python3.7 Change the way timeout is simulated - return error instead of raising ssl.SSLError
|
I ran the tests for both 3.7 and 3.6 and they work just fine 💯 |
koliber
left a comment
koliber
left a comment
There was a problem hiding this comment.
Code looks good.
This change bumps our version of the recurly lib, which is core to our billing integration. Please WIP this PR until the PPP squad can do regression testing on this new version.
| httpretty>=0.8,<=0.8.10 | ||
| recurly>=2.2.23 | ||
| httpretty>=0.9.4 | ||
| recurly>=2.9.0,<3.0.0 |
There was a problem hiding this comment.
This will require regression testing in the @15five/provisioning-payment-squad . We'll need to get a ticket set up for that.
koliber
changed the title
|
WIPed until @15five/provisioning-payment-squad tests the new version of the recurly lib. |
|
@koliber Thanks 🙇 FYI I remember doing regression testing on 2.8.6 and all was fine, definitely we don't want to go above 3.0.0 as it's introducing another version of API. |
|
I created https://15five-dev.atlassian.net/browse/ENG-9441 to do more in-depth billing testing. Is 2.8.6 the minimal we need to go to? The mocurle requirements state 2.9. Please update https://15five-dev.atlassian.net/browse/ENG-9441 to communicate the lowest version that is required for python 2.7. |
|
@koliber sorry for not being specific - I tested the 2.8.6 when I was implementing support for custom fields in Recurly a while back. For this, we need at least 2.9.0, but the best would be to go with the most recent 2.9.x version. I will update the ticket. |
pgrzesik
force-pushed
the
python3.7-compatibility
branch
from
2fe4c49 to
cb2bf8b
Compare
|
I put in a PR here: https://github.com/15five/fifteen5/pull/28358 that makes one change to PPP code necessary to move to recurly 2.9.16. It's got notes on my test process--if I've missed anything obvious I'm hoping the review will pick them up. |
|
As the tests were successfull, I'm merging this one |
pgrzesik
changed the title
|
Is it okay if we open-source this and submit a PR to Captricity#21? |
|
@caleb15 I would love to - the only thing I worry about is the fact that it kinda breaks/changes the approach on one of their functionalities (simulating the timeouts) - I'm not sure if they have docs but we should probably also update them if possible afterwards. cc @koliber what do you think about us providing a PR to |
|
I updated https://github.com/15five/fifteen5/pull/27702 to use the new mocurly code and the python tests are passing now! :D https://circleci.com/gh/15five/fifteen5/162673 I've also deployed the new code to cloud51 https://circleci.com/workflow-run/86924c3b-acf7-4acc-bab5-75d08b973522 |
|
I think it's a great idea to provide a PR with this update to mocurly. I somehow missed this thread. Apologies for replying so late. Thank you @caleb15 for drawing my attention back in. |
4 participants
Changes with reasoning:
httprettybecause previous version did not support Python3.7recurlybecause previous versions did not support Python3.7iso8601httpretty. Related issue Fix compatibility with later versions of httpretty Captricity/mocurly#13. I chose the version with just returning 502, which to be fair, makes way more sense to me than indicating timeouts viaSSLError. I checked in our codebase and it seems like we're not using that at all, but it's worth remembering as someone might get caught by that.