Closed Marcono1234 closed 9 months ago
@Marcono1234 The changes to dist/index.js
make the PR very difficult to merge. Can you please update the commit to remove that change?
Thanks for the feedback! I will try to adjust this in the next days.
Would it make sense though to use a plaintext / custom text format where for example each line represents a checksum, except if it is blank or starts with #
? For example something like:
# 1.0
87e50531ca7aab675f5bb65755ef78328afd64cf0877e37ad876047a8a014055
# 1.1
22c56a9780daeee00e5bf31621f991b68e73eff6fe8afca628a1fe2c50c6038e
# 1.2
5c91fa893665f3051eae14578fac2df14e737423387e75ffbeccd35f335a3d8b
...
JSON has the disadvantage that it doesn't support comments, and just having a large list of checksums without any indication to which versions they belong might make reviewing it difficult.
However, with #145 maybe it would make sense to use JSON instead of a custom text format, but then include the version numbers. For example:
{
"87e50531ca7aab675f5bb65755ef78328afd64cf0877e37ad876047a8a014055": [
"1.0"
],
"22c56a9780daeee00e5bf31621f991b68e73eff6fe8afca628a1fe2c50c6038e": [
"1.1",
]
}
However, with https://github.com/gradle/wrapper-validation-action/pull/145 maybe it would make sense to use JSON instead of a custom text format, but then include the version numbers.
Yes that's what I meant. Something like:
[
{ "version": "1.0", "checksum": "....." },
{ "version": "1.1", "checksum": "....." },
... etc ..
]
That way you could use JSON.parse
to read this directly into an array of Typescript object with a version
and checksum
attribute. Take a look at the links I posted for examples.
Once you have this array, you can get the list of checksums on their own using arrayOfObjects.map { it.checksum }
.
I have performed the following changes now:
path.resolve(__dirname, ...)
. This seems more reliable to me because depending on whether unit tests or the action (from index.js
) is run __dirname
differs (I assume), and therefore both src/...
and dist/...
would need to have the same nesting level for this to work.
I am not sure though if importing the JSON as module could become a performance problem (possibly during development only).KNOWN_VALID_CHECKSUMS
to be a Map<string, Set<string>>
, mapping from checksum to set of version names. This is for compatibility with #145; could then for example calculate the checksum for the JAR, and check whether the expected version is in the set for that checksum.I hope that is ok like this. Feedback is appreciated!
Thanks @Marcono1234 . I'm not going to have capacity to take this further until Feb 13 at the earliest. But it's on my list!
This has been merged manually. I need to find a better process for merging external PRs.
'd like to include this in the first RC of v2.0.0, since that mitigates some of the risk of the change. (People have to explicitly update to get the new version).
Actually, I forgot that I already released v2.0.0-rc.1
🤦🏼 .
This change will need to wait for a v2.1.0
release.
@Marcono1234 seriously, thank you so much for working this problem out. Really truly. You can see by the number of issues that @bigdaz closed how much pain our network connection logic has caused over the years.
You've just significantly increased the stability and usability of this action for all of the users of it.
Truly, thank you so much for your contribution. I'm incredibly appreciative you took the time to contribute it.
Thanks for the kind words and the feedback!
I have addressed the feedback (adding tests, and bot/
branch prefix) in #178.
I've just released v2.1.0
containing this change. Thanks again!
Fixes #161
If a checksum is not found in the hardcoded list, the action falls back to fetching the checksums from the Gradle API, as before.
For now there is no logic for automatically updating the list of known checksums; that has to be done manually. But maybe it could be automated in some way in the future.
Here is my somewhat hacky (but hopefully bug-free) Java code which I used to generate the list of checksums:
Checksums list creator
```java import java.io.IOException; import java.lang.reflect.Type; import java.time.Instant; import java.time.format.DateTimeFormatter; import java.util.Comparator; import java.util.HashSet; import java.util.List; import java.util.Locale; import java.util.Set; import com.google.gson.Gson; import com.google.gson.JsonDeserializationContext; import com.google.gson.JsonDeserializer; import com.google.gson.JsonElement; import com.google.gson.JsonParseException; import com.google.gson.annotations.JsonAdapter; import com.google.gson.reflect.TypeToken; import okhttp3.OkHttpClient; import okhttp3.Request; import okhttp3.Response; public class GradleChecksumsFetcher { private static final OkHttpClient client = new OkHttpClient(); private static String fetch(String url) throws IOException { Request request = new Request.Builder() .url(url) .build(); try (Response response = client.newCall(request).execute()) { return response.body().string(); } } static class VersionData { String version; String wrapperChecksumUrl; @JsonAdapter(BuildTimeAdapter.class) Instant buildTime; boolean snapshot; boolean nightly; boolean releaseNightly; private static class BuildTimeAdapter implements JsonDeserializer:warning: I am not that familiar with TypeScript, so any feedback is appreciated!