Skip to content

Conversation

@rayluo
Copy link
Contributor

@rayluo rayluo commented Sep 30, 2021

Even after this change, our CI/CD pipeline is still not yet picking up the latest cryptography 35.0.0, not until our upstream dependency PyJWT also makes similar change.

Thanks @beliaev-maksim and @graingert for bringing this to our attention.

This would close #411, close #413.

@beliaev-maksim
Copy link

@rayluo
I am fine to close my PR, just want to mention that it is not the way, how this is managed on GitHub. If you want a change in PR, you explicitly request change, eg I want up bound to be +3, but not opening another PR with same change

@rayluo
Copy link
Contributor Author

rayluo commented Sep 30, 2021

@rayluo I am fine to close my PR, just want to mention that it is not the way, how this is managed on GitHub. If you want a change in PR, you explicitly request change, eg I want up bound to be +3, but not opening another PR with same change

Thanks Maksim for your remind. We should have properly acknowledge your (and Thomas's) contribution, no matter what. My bad. Now we added the attribution in this PR's description.

We understand, and we do usually follow that convention of notifying the PR author to make some small change, because it would be convenient for both sides. But in this particular case, our proposed change AND its (imho equally important) inline comments would literally overwrite your entire existing PR. In such a case, we feel like it would seem bureaucratic to say "hey please copy and paste these 4 lines as-is into your existing PR: line 1... line 2... line 3... line 4...". We thought we would better use the same amount of typing to create a new PR, and saving you from a perhaps-boring copy&paste.

Thanks again. Meanwhile:

@rayluo rayluo merged commit 06c9cff into dev Oct 1, 2021
@rayluo rayluo deleted the bumping-cryptography-upper-bound branch October 1, 2021 21:51
@rayluo rayluo mentioned this pull request Oct 1, 2021
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.

3 participants