Reinstate SignedJwtAssertionCredentials API for compatibility with old code #687
Conversation
|
I would rather not, not without an extremely compelling reason. We made a major release to signal the change in the API surface. Downstream packages or projects should pin dependencies if they depend on older versions. |
|
Jon, most people really don't know or care about the internals of Google Service Account authentication. A lot of people do however have code they either they wrote, or that they have inherited. When you get code that you've inherited, and you need to run it in a new environment, it's pretty sucky to need to go and try to find an old version of a particular library, especially one as gnarly to install as this one. Sure one could go an update one's local code to match the new API, but then you need to manage an incompatible version upgrade across your own stack. I would strongly suggest that a breaking API surface change (such as this one) needs a compelling reason to be done in the first place, and in this particular case, it really isn't obvious what that is. The new class is essentially equivalent to the old one, albeit with a different message signature. (and unfortunately not everyone has the benefit of a big monolithic code repository where they can track down and change all their clients usages of an API before an update) |
|
Why can't those people just pin to an older version of |
|
Hi Danny, Not everyone has their dependencies vendored into source control - or even an automated build environment. There are still a lot of pet VMs and servers out there. I agree there are better ways to manage the client side but not everyone is there yet or ready to make the investment to get there. So when one of my clients build a new system, they go through the list of software to be installed. From the comments in the main bug, it seems pretty clear that they're not unique that regard either. But really, even with the best vendoring system in the world, why does this one need to be a breaking API change? To be clear, I'm all for updated and better interfaces, so yes please do ship the new one - but is it really that hard to keep a few lines of code around that wrap the old call to call the new one? (rather than force spreading the pain to each existing user of the library) Cheers, Adam |
|
No vendoring needed, this is why we version our software. The change was made at 2.0.0 (and this and "every" other library has always reserved the right to make breaking changes on a version bump). All the old versions are still there: https://pypi.python.org/simple/oauth2client/. In particular: |
|
Hi Danny, of course it's your library to choose when and where you want to make incompatible API changes. My input is that is that this one wasn't necessary to make. I think the mistake that many people make is assuming that their library or tool is the focus of the what their users are working on. If it was, then sure, no big deal to make a change to deal with an update. But when it's completely tangential from their core business, it's a real PITA to have to deal with an incompatible change like this. I didn't go looking for this one today, I was simply trying to help a client get better control over ~200 Python scripts they use for various automation tasks across their fleet of servers. Today's exercise was simply to get them to a clean While as you say I could have just figured out how to install the old version of oauth2client on my workstation, since it wasn't obvious to me how to do so, and since I found the bug with many others struggling with the same issue, I thought it would be productive to open a PR to the library to save others the same effort. Give the very long history of users having difficulty with SignedJwtAssertionCredentials (which if you recall, I think I have the dubious honour of opening pull request #1 in this same project in a previous attempt to address other aspects), I am very surprised to see a decision to rename the class be decided to be a good enough reason to break folks. Please don't underestimate the multiplying effect of any API surface change across your user base. Anyway, that's just my opinion, and I'm sure you're not short of others. :) For now we'll write some kind of shim on our end. Have a good holiday break! Cheers, Adam |
But it was, and that's why we made it. This library is insanely complicated for what it does, and we carefully consider every change. Before 2.0.0 there were two service account credentials classes with weird behavior.
It wasn't just a rename, it was a significant change in behavior and fixing a fundamentally broken set of classes. We did the right thing by bumping the major version number when we did this.
We don't, which is why we've taken great care to follow versioning guidelines and bump the major version number when we break API compatibility. This PR won't make this library compatible with its pre-2.0.0 interface. We also moved a ton of stuff into This is also superfluous because I'm working to deprecate this library as soon as possible. We appreciate your effort and feedback @aeijdenberg, but I don't think this is right solution to this problem. |
Please consider this pull request to re-add the old method signature for creation of
SignedJwtAssertionCredentials.There is a lot of old code out there that uses it (I came across some on a project today that was broken because of this) - and since the new class is essentially equivalent to the old, it would save a lot of work for a lot of people to have the core library handle the name change for them.