v2.0.0-alpha-1 🌈

[bitwiseman]

Major version bump to v2.x

I would love for github-api v2.x to be a larger change that has major new features.

However, the most pressing need is clearing technical debt for better stability and ease of adding new features. These changes break binary compatibility requiring a major version bump.

v2.0 includes:

• Drop support for Java 8
• Remove functional dependencies on HttpURLConnection
• Remove most deprecated classes and methods
• Remove all "bridge methods"

Changes

• Prepare release (bitwiseman): github-api-2.0.0-alpha-1 @bitwiseman (Prepare release (bitwiseman): github-api-2.0.0-alpha-1 #1936)
• Begin 2.x release train @bitwiseman (Begin 2.x release train #1935)
• Prepare release (bitwiseman): github-api-1.325 @github-actions (Prepare release (bitwiseman): github-api-1.325 #1932)

---

This discussion was created from the release v2.0.0-alpha-1 🌈.

[bitwiseman]

@gsmet @ihrigb @mestebangutierrez @tginiotis-at-work @kgromov @MarkEWaite @alecharp
Any feedback you can provide would be appreciated.

[gsmet]

@bitwiseman I was finally able to do some testing. From my perspective, it's all good: I ported Quarkus GitHub API and Quarkus GitHub App to it with only very minor changes and tested our GitHub bot both in JVM mode and native.

Only one thing comes to mind: it seems you dropped the unbridged artifact? I think you should reinstate it even if for now you don't have bridges as you will add some at some point and they are going to be problematic again for Mockito users.

[gsmet]

Oh and also, it seems you didn't drop all the IOException that are extremely annoying when coding with the API. It has been one of my major complaint over time, as it's a bit painful to have to throw IOException everywhere.

In some cases, we even have IOException for methods that are definitely not throwing anything such as:

github-api/src/main/java/org/kohsuke/github/GHDiscussion.java

Lines 42 to 44 in 7150121

	public URL getHtmlUrl() throws IOException {
	return GitHubClient.parseURL(htmlUrl);
	}

I think we have a unique opportunity here to clean this up.

[gsmet]

BTW, I'm aware it's going to require app changes but really it's for the better and given we are doing a major, I think it's probably now or never. People who don't want to handle this in their apps can keep using the old version if they prefer.

[bitwiseman]

Only one thing comes to mind: it seems you dropped the unbridged artifact?

There was a glitch that I noticed a few weeks after standard release of 1.325. I believe I fixed it with bfa107f and 19ba058 .
Maven Central has 1.325 and 1.326 under https://mvnrepository.com/artifact/org.kohsuke/github-api-unbridged .

Oh and also, it seems you didn't drop all the IOException that are extremely annoying when coding with the API.

This was intentional. As note it is disappointing, but I'm trying to walk the line for minimal number of binary breaking changes for the sake of Jenkins usage. Jenkins' plugin behavior means that compile-time breaking changes are less problematic than binary breaking changes - thus usage of bridge methods. Looking at japicmp - METHOD_NO_LONGER_THROWS_CHECKED_EXCEPTION it appears that removing checked exceptions is a compile-time breaking change but not a binary breaking change. If verified, then I'd be very comfortable removing most checked exceptions as part 2.0.

[gsmet]

If verified, then I'd be very comfortable removing most checked exceptions as part 2.0.

I think it would be worth checking. We had quite a lot of complaints about that over time and every time I demo the API at a conference, it does look unnecessarily verbose (which might seem like a small price to pay but we are all struggling to make our API more user friendly and it's definitely worth it).
It's also sometimes quite annoying when dealing with the Stream API and having to catch the exceptions in the lambda. I got hit by that several times and see #1658 for other complaints related to it.

[bitwiseman]

@gsmet
All the references I'm seeing say removing checked exceptions doesn't break binary compatibility.

[MarkEWaite]

From @MarkEWaite :

I think it is a mistake to sacrifice or significantly disrupt the existing users of the library in order to make the API better for users that are not yet consuming the API. I guess that if the disruption happens and it is too great, then the downstream users (like the Jenkins plugin) can fork the repository and maintain their own fork, but that will reduce the consumers of this API.

I'm not sure I understand what you mean. This change will improve the life of the existing users

I'd extend your phrase "This change will improve the life of the existing users" to say "This change will improve the life of the existing users if and only if they rework the logic in their code to adapt to the changes". In other words, existing users only benefit if they rework the logic in their code. That's what I meant when I said "sacrifice or significantly disrupt the existing users of the library".

then the downstream users (like the Jenkins plugin) can fork the repository and maintain their own fork, but that will reduce the consumers of this API.

They can continue to use the version they are using if they are happy with it. Or they can move to the new version and benefit from the better DevEx.

Being locked on a specific outdated version has been a bad experience in the Jenkins project. It creates a situation that looks like a fork of the upstream repository without the clarity of actually forking the upstream repository and releasing it as a new artifact.

When a security issue is detected in the upstream library, the consumer of the specific outdated version must either perform all the work to update to the fixed version (adapting to the behavior change while also performing a security update) or they must fork the repository and backport the security fix to a new release that is based on the specific outdated version.

[gsmet]

I understand your point but what you're saying is basically "never do a new major version because changes to the API will be too disruptive for the existing ecosystem".

I'm not saying the situation is ideal, it's definitely not. This change is quite nasty. But it's a bit of a shame that even for a major version, we can't do breaking changes when something is actually bad in the API.
Note that I have quite a lot of code using the GitHub API so I know it's going to have an impact on me too. I'm not exactly excited about it but I think this API would be a lot easier to use in the future if we did this change.

I agree it's going to be painful for a while but it might be worth it if it makes the life of our future selves and all the new users better.

When a security issue is detected in the upstream library, the consumer of the specific outdated version must either perform all the work to update to the fixed version (adapting to the behavior change while also performing a security update) or they must fork the repository and backport the security fix to a new release that is based on the specific outdated version.

That's why you usually push security fixes to multiple release streams - at least when they are not too disruptive.

Again, I won't be the one making the decision and I understand your point, I'm just trying to challenge things a bit because well a major version doesn't happen very often and they are usually quite unique occasions to improve things significantly.

[Haarolean]

I agree with Guillaume, if we can't afford to introduce breaking changes even in semver release versions, then, when can we?

[gsmet]

BTW, if we decide to go with it, I think we probably need to review very carefully the typology of the changes, categorize them and make sure the semantic is good and consistent for all of them.

I'm preparing Devoxx and will be off all next week but happy to discuss it further after Devoxx.

[bitwiseman]

Major version != free pass for all breaking changes

I understand your point but what you're saying is basically "never do a new major version because changes to the API will be too disruptive for the existing ecosystem".

No, what's being said is that just because a particular breaking API change has advantages doesn't mean it must be included in a major version bump.

I agree with Guillaume, if we can't afford to introduce breaking changes even in semver release versions, then, when can we?

Not all major versions are huge changes. The main reason I'm pulling the trigger on 2.x is to reduce ongoing maintenance headaches: Java 8, URLConnection, and ancient bridge methods. A major version bump is is not justification in itself for any particular change, it only means more changes can be considered.

Streaming-friendly API is an important feature, now how to implement it

I have not heard anyone speak against having a Streaming-friendly API. The only thing I've said is that it can be implemented without breaking existing contracts. Yes, that will add code to the project, but from my perspective that code has low complexity. I'm even comfortable saying we should move to have streaming-friendly code being the default for new features. We progressively deprecate the old code and move the project in that direction so that in a year or so we can cut a 3.0 that drops the old style.

Not all API problems related to exceptions are the exceptions fault

This method is a good example of having to handle simple logic with exception control flow: https://github.com/quarkusio/conversational-release-action/blob/main/src/main/java/io/quarkus/bot/release/step/CreateBranch.java#L111 . I'm not sure it actually helps making things readable and knowing what is a normal error case (this thing doesn't exist) and what is not (I was unable to contact GitHub). It's definitely not fluid when writing code (and having to refer to the Javadoc for every call you write is definitely not ideal).

Especially since I would expect an exception only if for instance I was unable to contact GitHub, not if the thing doesn't exist. The fact that a label doesn't exist is a perfectly valid case from a Java API POV. It shouldn't be an error.

Ah! This is particularly interesting case. Yes, this should be an exception - you asked for an instance of a specific branch (or label, or whatever) and none was returned. The API doesn't know that you want to say is, "If branch A doesn't exist create it" or "If label A exists, rename it. What we should be talking about is not removing exceptions, but making a more expressive API overall. For instance, anything that has a get* or a list* should automatically have an exists* method. But the way the API is written currently makes that a pain to implement. What if your code could look like this:

if (!repository.branch().name(releaseInformation.getBranch()).exists()) {
    try {
        // If get() fails, we want an exception, not a null.
        String sha = repository.branch().name(releaseInformation.getOriginBranch()).get().getSHA1();
        repository.ref().create()
            .name("refs/heads/" + releaseInformation.getBranch())
            .sha(sha)
            .done();
    } catch (Exception e2) {
        throw new IllegalStateException(
                "Unable to create branch " + releaseInformation.getBranch() + " from "
                        + releaseInformation.getOriginBranch() + ": " + e2.getMessage(),
                e2);
    }
}

Rather than getting stuck on modifying the existing methods, let's talk about creating an API that is better.

[gsmet]

No, what's being said is that just because a particular breaking API change has advantages doesn't mean it must be included in a major version bump.

No, sorry, that's not what is being said by some people in the discussion and I was specifically referring to that (sorry, no nested threading creates confusion).
The same "it will break existing users" and "we can't stay on an old version either" arguments made will be there for version 3 or 4 if we don't have a clear plan on how to get there.
We are talking about a library that could be there for the next 20 years. It will have to evolve and there will be major breaking changes.

Not all major versions are huge changes. The main reason I'm pulling the trigger on 2.x is to reduce ongoing maintenance headaches: Java 8, URLConnection, and ancient bridge methods. A major version bump is is not justification in itself for any particular change, it only means more changes can be considered.

That's exactly what we are asking here. That we consider this change for the new major version.
And that we discuss how it could make the API better in the various cases there is.

For me there are two different discussions:

• what is the API we want if we didn't have the history?
• how do we get there (if there's a way to get there)? Answer might be we use another groupId. Answer might be we don't.

If we don't have an honest discussion about the first point, I'm seriously in doubt we will be able to significantly improve ever.

Now if your goal is to release version 2 quickly and that you are not closed to the idea of discussing another major release soonish, that makes sense. But if the idea is that we make a new major every 5 years, then we know that we are stuck for another 5 years.

Our understanding was that, given we were preparing a new major, we could take a deep breath and discuss where we want to go but that might have been our mistake if it wasn't the goal of this version 2.

As the maintainer, it's really your call what you want to do with this library. But as contributors, it's important for us to know what the general plan is to see where we put our efforts.

Ah! This is particularly interesting case. Yes, this should be an exception - you asked for an instance of a specific branch (or label, or whatever) and none was returned. The API doesn't know that you want to say is, "If branch A doesn't exist create it" or "If label A exists, rename it. What we should be talking about is not removing exceptions, but making a more expressive API overall. For instance, anything that has a get* or a list* should automatically have an exists* method. But the way the API is written currently makes that a pain to implement. What if your code could look like this:

I really don't see your proposal as an improvement API wise but I suppose it's a matter of taste. You are adding a lot of verbosity/specificity to something that is solvable with standard Java constructs.

Also if I want to do something like getting the branch or creating it if doesn't exist then do something, I will have one more request in the former case, which is not ideal when being rate-limited. Or you will end up with an Optional basically.

BTW, this is exactly what I was asking for earlier, that we try to categorize cases and see how we would like to solve them. Then we can discuss how breaking it is and how we want to get there.
We could also have a look at how other GitHub APIs do it.

There are cases where we know we just want to drop the IOException (see the getHtmlUrl() case I pointed out), others where we probably want to but that are less obvious and then some for which we will need to think about what we actually want to express.

[bitwiseman]

@gsmet
This has been a long thread with strong opinions expressed by all parties.

That's exactly what we are asking here. That we consider this change for the new major version. And that we discuss how it could make the API better in the various cases there is.

You seem to think that I'm not giving your proposals their proper consideration. I've already devoted a chunk of time to creating a prototype for the "remove all checked exceptions" that folks have suggested. Yes, my determination from that was to decline that particular proposal for this version, but I put in the good-faith effort to evaluate it. I am listening to your requests and I'm putting the work and I am open to other proposed changes.

Now if your goal is to release version 2 quickly and that you are not closed to the idea of discussing another major release soonish, that

Yes, my goal (perhaps not clearly stated enough in the descript of this release) is to release version 2 quickly. I am complete open to making another major release in 6-18 months.

I would be very comfortable agreeing to a road map where:

• a new API is implemented in a separate package in this library
• elements of the old (current) API are deprecated as the functionality is implemented in the new API
• 6-12 months after the new API achieves feature parity with current API, we cut a major version removing the old API

...
For me there are two different discussions:

• what is the API we want if we didn't have the history?
• how do we get there (if there's a way to get there)? Answer might be we use another groupId. Answer might be we don't.

So, what are your proposals for these two topics? Especially the first one.
What would you like https://github.com/quarkusio/conversational-release-action/blob/main/src/main/java/io/quarkus/bot/release/step/CreateBranch.java#L111 to look like ideally?

BTW, this is exactly what I was asking for earlier, that we try to categorize cases and see how we would like to solve them. Then we can discuss how breaking it is and how we want to get there. We could also have a look at how other GitHub APIs do it.

There are cases where we know we just want to drop the IOException (see the getHtmlUrl() case I pointed out), others where we probably want to but that are less obvious and then some for which we will need to think about what we actually want to express.

Please create an issue to get us started (or add to the existing one, though it's a bit of a long with discussion already). I don't have the bandwidth to lead this charge.

[gsmet]

Hey Liam,

Sorry, things have been a bit crazy on my side.

I completely agree with you, I think most of the misunderstanding was about the goals of this version 2.

I'll work on a proposal as time permits, discuss it with my colleagues and try to draw what could be done for the future.

Cheers!

[bh-dremio]

Just wanted to send a big thank you for the effort in cleaning up the repo.

The removal of the bridge function annotations saved me a major headache with mockito unit testing.

[alecharp]

Thank you @bitwiseman for your effort on this. I'm sorry I haven't had time to try the alpha version so far. I'm catching up.
So far, migration from 1.318 to 2.0.0-alpha-2 is fine. Test migrations to a bit more brain consuming with listing pull requests as we have to deal with the PageIterable.

[alecharp]

I have to admit that I'm hitting a problem with the PageIterable.
I would like to be able to say that for any GHRepository#queryPullRequests()#list(), I return some content. I could precise the toList() or toArray() method, but for some implementation I could prefer to use the splitIterator() for a Stream implementation.

The problem is that the PageIterable implementations are usable, and implementing one is problematic as the PageIterator constructor is package visible only, so I cannot use it in a PageIterable#_iterator() implementation.

I don't have a solution right now or an idea of a solution but I'll try to think of one.

[bitwiseman]

@alecharp
I've thought about exposing page iterator as public. But I've been looking to coordinate it with other improvements to page iterable. See #1730 and related issues and discussion.

Can you give an example of what you'd like the API to look like?

[alecharp]

I tent to like the Stream API.
If we could have a .stream(), so that we can easily manipulate the feedback of the API. It could provide an equivalent of the .toList() or toArray() of the PageIterator.
It could simplify the mocking of such request, no?