fix: add connection timeout; remove legacy jenkins adapter #641
Conversation
Partially addressed #640
This introduces a new translation key `splash_fetchReleases`.
Fetching available releases from independent sources should be independent of each other. Thus, we can run this in parallel by using a `parallelStream()`.
The legacy Jenkins instance (`jenkins.terasology.org`) is no longer available. Therefore, we don't need this repository adapter anymore. Closes #640
skaldarnar
added
Type: Bug
|
After merging this we should make a new bugfix release and notify the user who reported the issue on reddit. |
This also makes the JenkinsClient testable again by making the `openStream` utility method a static member. This allows to stub only that static method for testing, while keeping the class-under-test untouched.
| return gson.fromJson(reader, Jenkins.ApiResult.class); | ||
| } catch (JsonSyntaxException | JsonIOException e) { | ||
| logger.warn("Failed to read JSON from '{}'", url.toExternalForm(), e); | ||
| } catch (IOException e) { | ||
| logger.warn("Failed to read from URL: {}", url.toExternalForm(), e); | ||
| logger.warn("Failed to read from URL '{}'\n\t{}", e.getMessage(), url.toExternalForm()); |
There was a problem hiding this comment.
I think you've gotten the argument order swapped here. You still want the url first,
and then if you want to just do e.getMessage() instead of the logger's default exception formatter, that's your second argument.
There was a problem hiding this comment.
(also, if you find yourself avoiding the logger's default exception formatter often, that could be a sign that we should reconfigure the logger)
There was a problem hiding this comment.
Hm, I'm actually not sure about best logging patterns. If you happen to know good resources on that let me know 🤓
The arguments are actually in the order I want them to be. The message is usually short and descriptive, so you can see what's wrong directly. The URL is pretty lengthy (maybe another option would be to drop the query parameters?), that's I wanted to move that to second line. Just forgot to adjust the text itself to reflect that.
On changing the default exception formatter: I do think that logging a full stack trace is only appropriate if we expect this is needed for investigating the issue, and/or if this is fatal error we cannot recover from. In this case, an exception here is unfortunate, but can happen for various reasons. Usually the stacktrace does not give helpful info here, but clutters the log.
There was a problem hiding this comment.
ah, I didn't expect the URL to be that long, but that would explain it
There was a problem hiding this comment.
For reference the full URL with search query parameters:
http://jenkins.terasology.io/teraorg/job/Terasology/job/Omega/job/master/api/json?tree=builds[number,timestamp,result,artifacts[fileName,relativePath],url]
Consisting of the base URL
http://jenkins.terasology.io/
followed by the job descriptor
/teraorg/job/Terasology/job/Omega/job/master
and finally a filtered view on the data
/api/json?tree=builds[number,timestamp,result,artifacts[fileName,relativePath],url]
So yeah, it's pretty lengthy 😅
| @@ -157,3 +157,4 @@ update_game_extractZip=Spiel-Datei entpacken ... | |||
| update_game_gameInfo=Spiel-Informationen aktualisieren ... | |||
| update_game_startDownload=Spiel herunterladen ... | |||
| update_launcher_updateFailed=Launcher-Aktualisierung fehlgeschlagen! | |||
| splash_fetchReleases=Rufe verügbare Spiele ab ... | |||
There was a problem hiding this comment.
👍 remembering to update the translation catalog!
| try (MockedStatic<JenkinsClient> utilities = Mockito.mockStatic(JenkinsClient.class)) { | ||
| utilities.when(() -> JenkinsClient.openStream(urlToInvalidPayload)).thenReturn(invalidPayload); |
There was a problem hiding this comment.
This is a little different than the other mockito usages I've seen. Is it because it's a static function, and mocking those works differently?
There was a problem hiding this comment.
Yep, exactly. I was looking at https://www.baeldung.com/mockito-mock-static-methods for reference.
I feel uneasy to mock (parts of) the unit under test, and using a static method that I can stub separately looked like a good compromise to keep the test.
This PR removes the legacy Jenkins adapter (for
jenkins.terasology.org) as we've shut it down recently.In addtion, it improves the handling of unavailable resources by introducing a 10 second timeout for the
connections in JenkinsClient. This will improve offline usage of the launcher, as well as cases where
the new Jenkins instance is not available.
Closes #640.