gradle / wrapper-validation-action

Gradle Wrapper Validation Action
https://github.com/marketplace/actions/gradle-wrapper-validation
MIT License
259 stars 58 forks source link

Update known wrapper checksums #175

Closed github-actions[bot] closed 9 months ago

github-actions[bot] commented 9 months ago

Automatically generated pull request to update the known wrapper checksums.

In case of conflicts, manually run the workflow from the Actions tab, the changes will then be force-pushed onto this pull request branch. Do not manually update the pull request branch; those changes might get overwritten.

[!IMPORTANT]
GitHub workflows have not been executed for this pull request yet. Before merging, close and then directly reopen this pull request to trigger the workflows.

Marcono1234 commented 9 months ago

@bigdaz, I am not completely sure why it picked you as author of the generated commit. Maybe because you merged the original changes and are now considered the person who triggered the scheduled workflow.

If that is not desired because it could cause confusion, we could edit the workflow and specify a different author, for example the GitHub bot account.

bigdaz commented 9 months ago

@Marcono1234 One downside of loading the JSON as a module is that we need to rebuild index.js on each update to wrapper-checksums.json. Not a blocker: I do think it's a cleaner implementation mechanism.

Ideally, we'd come up with some sort of "test-and-merge" workflow that would:

  1. Execute npm run all to generate index.js
  2. Run all of the tests
  3. Add a commit with the change to index.js
  4. Merge the PR
Marcono1234 commented 9 months ago

One downside of loading the JSON as a module is that we need to rebuild index.js on each update to wrapper-checksums.json.

This could probably be solved by letting the GitHub workflow not only update wrapper-checksums.json, but also execute npm run build afterwards to update index.js as well. Should I change that?


Ideally, we'd come up with some sort of "test-and-merge" workflow that would ...

Do you mean this should be executed right before the merge? I am not sure if that will be possible, since the index.js would then have to be committed on the branch of the PR author. But maybe I am misunderstanding you.

Maybe the approach of having a workflow for the main branch which updates index.js after every push could work? It might even be possible to add index.js to .gitignore so users don't commit it by accident for their PRs and then let the workflow run git add --force to override this (?).

bigdaz commented 9 months ago

This could probably be solved by letting the GitHub workflow not only update wrapper-checksums.json, but also execute npm run build afterwards to update index.js as well. Should I change that?

Yes, please. That would be excellent.

Do you mean this should be executed right before the merge? I am not sure if that will be possible, since the index.js would then have to be committed on the branch of the PR author. But maybe I am misunderstanding you.

This is what I meant. I think it could work as long as we require that all contributed PRs have "Allow edits and access to secrets by maintainers" checked.

Maybe the approach of having a workflow for the main branch which updates index.js after every push could work? It might even be possible to add index.js to .gitignore so users don't commit it by accident for their PRs and then let the workflow run git add --force to override this (?).

Yes, this could be really nice.

Marcono1234 commented 8 months ago

@bigdaz, I have been thinking a bit more about this.

Maybe the workflow for updating wrapper-checksums.json should not automatically update index.js, because then there might be a conflict for that file but Git is automatically able to 'resolve' it, even if the resulting index.js content would not be the same as if you had built it with npm run build. Instead, if possible, we could rely on whatever we implement for regular user-submitted pull requests to update index.js.

My suggestion to use .gitignore above to prevent users from committing index.js probably won't work because Git is already tracking the file, so Git will detect changes to it and report it to the user.

What could work is copying the index.js to a different directory after the build, and only tracking that with Git. For example:

Marcono1234 commented 8 months ago

Have created a proof of concept here: #187