-
Notifications
You must be signed in to change notification settings - Fork 17
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
Better api error handling #62
Conversation
Signed-off-by: StefMa <StefMaDev@outlook.com>
Signed-off-by: StefMa <StefMaDev@outlook.com>
Signed-off-by: StefMa <StefMaDev@outlook.com>
755063d
to
2555271
Compare
Thanks @StefMa. I keep wondering if it wouldn't be a better fix to catch the original exception on the Gradle side and provide a better error message... Because with or without your changes, the build has to fail; it can't really continue (it attempted to download a toolchain because there is no local alternative). I will get a second opinion internally. |
Currently it will fail.
And I guess this is the right approach. So this behavior is... correct?! Why should Gradle itself catch an exception in case an toolchain repository throws an error? I think Gradle shouldn't catch exceptions happen in that toolchain repos because it also don't catch exceptions in (normal) plugins. They will also be just thrown. Just my two cents on that 🙃 |
@StefMa after thinking a bit more about it, I propose we do the following:
I think the Gradle side change is necessary, because, if one resolver fails, then we still want to try subsequent ones, if they have been set. This fix will also offer a solution to build authors: they will now be able to set multiple resolvers (assuming the community writes some more) to have a backup when Foojay backend failures happen. WDYT? |
Hey @jbartok, I guess the propsed change on Gradle side makes a lot of sense. I also think that adding a retry mechanism to this plugin makes sense. However, I also think it make sense to still add this fix to the plugin. Even though Gradle will catch exceptions, the goal of a toolchain provider should be to provide an I guess the fix here and the fix in Gradle are separated things. Both fixes strengthen the toolchain support in Gradle. |
I'm not sure we want to blur the distinction between "Fojjay has no matching toolchains in their repertoire" and "Foojay has crashed and burned". Will think about it. |
@StefMa, your reasoning makes sense to me, and I'm not 100% sure of myself when I say this, but let's go with JUST the Gradle side fix for now. We can always revisit this later if we change our minds. I will open a separate issue for the exponential back-off improvement to the plugin, in case somebody decides to implement it. |
All I wanted to fix is #47 .. suddently it ended up in a short rewrite 😅
fixes #47
Main fix is here:
Instead of throwing a
GradleException
in case API return != 200 status code, we return anResult.failure
(Kotlin builtin).This is how it look before this PR:
foojay-toolchains/foojay-resolver/src/main/kotlin/org/gradle/toolchains/foojay/FoojayApi.kt
Lines 107 to 109 in 15211a0
Now it looks like this
The result will be bubbled up until it reaches the
Resolver
.The Resolver will then do a logging (
warn
level) and return anOptional.empty
.The resulting message will then instead of one of the linked issue be like:
(Ignore the case that a java 18 Zulu package is available, for testing purpose I modified the result.success case to be an error 😉 )
This is the "default" error message that will be displayed by the Gradle build tool itself and I think it is more obvious (but more generic) than the one before.
Furthermore, if running with
--info
(or something) the output includesFailed to resolve Java toolchain
.Not sure if we want to enhance this? 🤔
We can 🤷 😁
This doesn't "fix the problem" in case of foojay api is down AND there is no local jdk installed.
In this case users are "locked in" and can't work until foojay api is up again.
But I guess providing a fallback is out of scope of this plugin ...
Maybe we can enhance the error message with "If foojay api is down, use one of the locally installed java versions instead: [listOfLocallyAvailableJdks]"
What do you think? 🤔
Whatever, about the rewrite.. 😁
I extracted a
FoojayService
out ofFoojayApi
.FoojayApi
has only the responsibility to make the connection and return anResult<JsonString>
.Thats it.
FoojayService
will handle thefindMatchingDistribution
, followed by thefindMatchingPackag
logic...I have the feeling this makes things a bit nicer ...