-
Notifications
You must be signed in to change notification settings - Fork 225
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
Remove deprecated methods #190
Conversation
We need to be very careful with removals in this library, and make sure the major libraries in the ecosystem have advanced to the version containing the new methods and have removed references to the deprecated ones. (We need to check grpc, google-cloud-java, and Beam). If we don't do this, this particular library could be a huge source of pain for diamond dependency conflicts. (It is very deep in the dependency tree.) I was almost thinking of recommend that these deprecated methods not even get removed when it is pushed to 1.0. |
This pre-1.0 library has had one release in the last year+, and only really works on App Engine standard. Now is the time to cut the dead wood, so dependents can upgrade at a more leisurely pace. For now, they can stick with 0.11 and have one PR that upgrades to 0.12 while fixing any deprecated methods. In a non-monorepo world we can't require all usages of methods to be changed at the same time we push a new release. The sooner we make the change, the easier it will be for everyone. |
This library is used for many cases:
This library works on-premise, in GCE, on GKE, etc. In a non-monorepo world, we can require that the major consumers become "eventually consistent" before the removal. This is essentially a two-phase commit. Why does making the change sooner make it easier for everyone? |
To upgrade those libraries one can either manually inspect all possible usages, or simply add a new version with the methods removed and see what breaks. Removing these post-1.0 would be an API breaking change that requires a new major version so we definitely don't want to wait till then to do this. And I'd like to get 1.0 out the door sooner rather than later. The libraries you cite are all GA or include GA artifacts, so depending on this is a problem. Looks like I misread: only google-auth-library-appengine depends on the App Engine SDK. |
You don't have to publish a new version to maven central to see what breaks; you can I completely agree that the dependency of GA libraries on unstable libraries is a problem - we both worked on https://github.com/GoogleCloudPlatform/cloud-opensource-java/blob/master/library-best-practices/JLBP-4.md , after all :-) |
We should check usage for these against artifacts that use |
@elharo, what's the status of this PR? |
We still need to verify that we aren't using any of these methods downstream (at least across Google controlled libraries) |
I haven't checked on this lately, so there may be new conflicts with this specific PR, but my opinion is still the same as when I sent it: remove all the deprecated methods now, before 1.0, and without worrying about who might be calling them. No project should be immediately broken by this since dependency upgrades are not automatic. If a project does attempt an upgrade and is broken, this will be a compile time failure and easily detected. |
Disagree. This library is "common-law GA". |
Fish or cut bait time. If this library is common-law GA, then remove the deprecation messages, commit to supporting those methods indefinitely, and publish a 1.0 version. The halfway state we're in now is confusing and unfair to library dependents. |
Completely agreed, we need to get the library out of its ambiguous state. |
Just to be clear, I don't have an informed technical opinion on the specific methods here. I do believe we should go to 1.0, and we should remove or undeprecate these methods before we do. |
@@ -193,7 +180,6 @@ protected GoogleCredentials() { | |||
* @deprecated Use {@link #create(AccessToken)} instead. This constructor will either be deleted | |||
* or made protected/private in a later version. | |||
**/ | |||
@Deprecated |
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 see @Deprecated
was removed here. If it's actively used, please also remove the @deprecated
comment`
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.
done
Who is the owner of the decision of whether or not this ought to be merged? |
Codecov Report
@@ Coverage Diff @@
## master #190 +/- ##
=========================================
Coverage ? 78.26%
Complexity ? 329
=========================================
Files ? 21
Lines ? 1445
Branches ? 158
=========================================
Hits ? 1131
Misses ? 236
Partials ? 78
Continue to review full report at Codecov.
|
Given lack of clear ownership, I'll take responsibility. There will potentially be subtle diamond dependency problems based on this change. The recommendation is to raise an issue in this repository, but also in the repository that's using old deprecated APIs. |
Immediately following the last release is the best time to rip out all the deprecated methods.