Allow configuration of HTTP timeouts for maven repositories #3951
Conversation
1c9d5b4 to
976f48c
Compare
976f48c to
3aaf687
Compare
| /** | ||
| * Constructor required by {@link org.openrewrite.gradle.marker.GradleProject}. | ||
| * | ||
| * @deprecated Use {@link #MavenRepository(String, String, String, String, boolean, String, String, Duration, Duration, Boolean)} | ||
| */ | ||
| @Deprecated | ||
| @JsonIgnore | ||
| public MavenRepository( | ||
| @Nullable String id, String uri, @Nullable String releases, @Nullable String snapshots, boolean knownToExist, | ||
| @Nullable String username, @Nullable String password, @Nullable Boolean deriveMetadataIfMissing | ||
| ) { | ||
| this(id, uri, releases, snapshots, knownToExist, username, password, null, null, deriveMetadataIfMissing); | ||
| } |
There was a problem hiding this comment.
This is used by an external project, org.openrwrite.gradle.tooling:model.
3aaf687 to
fef3f86
Compare
|
Nice addition @ammachado ! I'm tagging @sambsnyd for an additional review as we're adding fields to tree classes, and he reworked parts of the Maven repository handling recently. Also copying the details on the failed test to save others a click |
As a suggestion, can you consider publishing the test reports on a failed build? Perhaps using a custom action for it (for example, https://github.com/marketplace/actions/junit-report-action). |
Yes I'd been eyeing https://github.com/gradle/actions/tree/main/setup-gradle#build-reporting earlier today, as part of our upgrade in https://github.com/openrewrite/gh-automation/ . It's been a pain point of mine as well to have to go into the logs. |
|
What's the motivation here? Does this solve a problem you are running into? |
|
Not my PR but I am watching as I also have an issue that would be solved with this. During execution the plugin downloads |
That's exactly the issue that originated this change. |
|
Thanks a lot @ammachado ! Should help to conform to the user's configured session, as we've seen more variation in artifact repository performance than we had expected folks would tolerate in their day to day work. 🙃 |
| <configuration> | ||
| <connectTimeout>40000</connectTimeout> | ||
| <readTimeout>50000</readTimeout> | ||
| </configuration> |
There was a problem hiding this comment.
I'm having another look at https://maven.apache.org/guides/mini/guide-http-settings.html#connection-timeouts
Over there it seems there's two options
A. an older <timeout> directly in <configuration>
<settings>
<servers>
<server>
<id>my-server</id>
<configuration>
<timeout>6000</timeout> <!-- milliseconds -->
</configuration>
</server>
</servers>
</settings>B. a newer <connectionTimeout> and <readTimeout> nested inside <httpConfiguration><put>
<settings>
<servers>
<server>
<id>my-server</id>
<configuration>
<httpConfiguration>
<put>
<connectionTimeout>10000</connectionTimeout> <!-- milliseconds -->
</put>
</httpConfiguration>
</configuration>
</server>
</servers>
</settings>In this PR
We seem to have added connectTimeout (as opposed to connectionTimeout, as well as readTimeout, but neither of those nested in the <httpConfiguration> + <put> elements I'd expect from the Maven docs.
In practice I think these fields won't be set when reading a Maven settings file from disk in style A or B above. Should we make any adjustments here @ammachado ?
There was a problem hiding this comment.
…ite#3951) * Allow configuration of HTTP timeouts for maven repositories * Applying server connection timeouts to `MavenRepositoryMirror`. * Reverting unnecessary changes * Increasing timeouts due to flaky connections * Reusing repository settings when using profiles --------- Co-authored-by: Tim te Beek <tim@moderne.io>
What's changed?
Changed maven downloader to allow configuration of connection/read timeouts
What's your motivation?
https://maven.apache.org/guides/mini/guide-http-settings.html#connection-timeouts
Anything in particular you'd like reviewers to focus on?
No
Anyone you would like to review specifically?
No
Have you considered any alternatives or workarounds?
No
Any additional context
N/A
Checklist