Skip to content
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

Introduce configuration option to detect gradle versions from gradle-wrapper.properties #145

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mikepenz
Copy link

@mikepenz mikepenz commented Aug 24, 2023

I do understand that this PR does not match the "contribution guideline" specifically on the requirement for:

It is not the goal of this action to verify that the gradle-wrapper.jar matches a specific version of Gradle, nor that the version declared in the build.gradle or gradle-wrapper.properties file matches.
https://github.com/gradle/wrapper-validation-action/blob/56b90f209b02bf6d1deae490e9ef18b21a389cd4/CONTRIBUTING.md#non-goals

However, seeing related issues still being kept open - and overall comments indicate positive interest towards such a functionality (E.g. #96 (comment), gradle/actions#286)

It would be great to see this functionality being added as I believe this will speed up the validation as a whole, but also lower the network utilization. Especially as recently the occurrences for request timeouts seem to increase (with builds frequently failing) - Related tickets # 46, # 40,


This PR introduces a new configuration option (which is disabled by default) called detect-version.

When enabled the action will now try to retrieve the gradle version associated with a gradle wrapper via the distributionUrl in the gradle-wrapper.properties.

Furthermore, it adjusts the checksum fetching condition to only retrieve checksums for the detected versions (instead of fetching versions for all possible releases).

This PR aims to fix to open issue: #96

However, while it was not specifically designed for this purpose it resolves partially gradle/actions#282 as it will only include checksums for versions detected.
A future update could include that we retain wrapper <-> version && checksum <-> version details to only validate the specific combination.

Additionally, I believe that the initial discovery and parsing of the gradle-wrapper.properties unlock and enable future expansions to for example handle: gradle/actions#286


The PR includes the relevant changes to the action and updated test cases to cover the new functionality.

@Marcono1234
Copy link
Contributor

Marcono1234 commented Nov 7, 2023

it partially resolves gradle/actions#282

GitHub detected the "resolves #..." as keyword and linked the issue, so it would be closed if this PR here is merged. I assume this is not intended since as you said it only partially resolves it.
This can possibly be solved by simply rewording it, for example from "partially resolves #..." to "resolves partially #...".

@mikepenz mikepenz force-pushed the feature/detect_versions branch from 3b03a0a to 27b277e Compare January 25, 2024 08:51
@bigdaz
Copy link
Member

bigdaz commented Jan 30, 2024

@mikepenz apologies for the extended delay responding to this. I'm notionally responsible for maintenance of this action, but haven't had time to make it a priority.

@bigdaz
Copy link
Member

bigdaz commented Jan 30, 2024

The changes to dist/index.js make the PR pretty much impossible to merge. Can you please update the PR, removing this change? It's easy for me to regenerate the outputs just prior to merging.

@mikepenz mikepenz force-pushed the feature/detect_versions branch 2 times, most recently from 7459b78 to 1fb4000 Compare January 30, 2024 07:12
@mikepenz
Copy link
Author

@bigdaz no worries. Thank you for having a look at it.
I rebased the branch on top of the latest main and removed the pre-compiled dist/index.js file.

action.yml Outdated Show resolved Hide resolved
@bigdaz
Copy link
Member

bigdaz commented Feb 7, 2024

@mikepenz @Marcono1234 @JLLeitschuh With the change to include hardcoded checksums, do y'all feel that this is still desirable? I haven't had time to review these changes.

It seems that the upgrade to Node 20 may have made the network situation worse, so my current thinking is that we promptly release v2.1.0 with only the hardcoded checksums fix, and consider this PR in the following release.

Further, has this PR been updated to take advantage of the hardcoded checksums?

@JLLeitschuh
Copy link
Contributor

It seems that the upgrade to Node 20 may have made the network situation worse, so my current thinking is that we promptly release v2.1.0 with only the hardcoded checksums fix, and consider this PR in the following release.

Oof, well in that case, absolutely cut a release ASAP.

Happy to consider this PR later

@mikepenz
Copy link
Author

mikepenz commented Feb 7, 2024

I rebased this PR before the hardcoded checksum PR was merged. Need to check again as it appears to be in significant conflicts now.

@bigdaz
Copy link
Member

bigdaz commented Feb 7, 2024

Oof, well in that case, absolutely cut a release ASAP.

Yep, see these comments from @deejay1: #174 (comment)

@Marcono1234
Copy link
Contributor

Marcono1234 commented Feb 7, 2024

@mikepenz, the new KNOWN_VALID_CHECKSUMS is a Map<string, Set<string>>, mapping from checksum to a set of version names. This should hopefully allow integration with your changes here. You could then for example calculate the checksum for the JAR, and check whether the expected version is in the Set for that checksum.
Though if the Map does not contain an entry for it, or if the Set does not contain the version, you would still need to fetch the checksum from the API since new Gradle versions might have the same wrapper checksum as previous versions.

@JLLeitschuh
Copy link
Contributor

When enabled the action will now try to retrieve the gradle version associated with a gradle wrapper via the distributionUrl in the gradle-wrapper.properties.

I'm concerned about relying upon this generally.

In my experience, many people don't know how to use the wrapper task properly. In order to actually update the gradle-wrapper.jar file, you actually need to run the wrapper task twice, the first time to update the gradle-wrapper.properties, and the second time to update the gradle-wrapper.jar. Many don't so the version in the gradle-wrapper.properties is actually one version ahead of whatever version of the gradle-wrapper.jar is being used.

Given this, I'm concerned that this change will significantly increase the false-positive rate of this action, which will decrease user trust in it.

If the hard-coded checksums solve the problem, I'd be more inclined to see if that solves most of the pain before we attempt including this strategy as well.

If we were to move forward with this strategy, I'd advise that we instead use the distributionUrl as a starting point, and then perhaps do a search starting with the distributionUrl version as a starting point.

…detected versions

  - version is detected from gradle-wrapper.properties
  - checksum is only fetched for these particular versions
- FIX gradle#96
- update action.yml with new config option
- update and introduce testcases for the new configuration option
@mikepenz mikepenz force-pushed the feature/detect_versions branch from b1516fa to ba65536 Compare February 23, 2024 16:52
@mikepenz
Copy link
Author

Thank you @JLLeitschuh for looking into this PR.

I rebased it just in case, however feel free to close it, given the hardcoded checksum solution may solve it now.

It may be valid to re-work this PR to offer a "strict" mode which would be more restrictive than just accepting any hash from the whole set. With the intention to force users to have the correct hash for the correct version. With some more messaging to inform that the double wrapper update may be the cause of the issue.

Copy link

@nooboobhuh nooboobhuh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UJI

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants