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

Update known wrapper checksums #175

Merged
merged 1 commit into from
Feb 7, 2024
Merged

Update known wrapper checksums #175

merged 1 commit into from
Feb 7, 2024

Conversation

github-actions[bot]
Copy link

@github-actions github-actions bot commented Feb 3, 2024

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
Copy link
Contributor

Marcono1234 commented Feb 3, 2024

@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 bigdaz merged commit c4ee1cd into main Feb 7, 2024
@bigdaz bigdaz deleted the wrapper-checksums-update branch February 7, 2024 03:57
@bigdaz
Copy link
Member

bigdaz commented Feb 7, 2024

@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
Copy link
Contributor

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
Copy link
Member

bigdaz commented Feb 9, 2024

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
Copy link
Contributor

Marcono1234 commented Feb 22, 2024

@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:

  • Remove dist/index.js from Git (respectively rename it, see below)
  • Create workflow which on every push to main builds the action, then copies dist/index.js to action/index.js.
    Only the workflow performs this copying, there is no task for it in package.json to prevent users from accidentally running it.
    Edit: There is a task (to allow reusing this by the GitHub workflow integration tests), but its name clearly indicates that this is for GitHub CI, and it might only work for Linux.
  • Replace check-dist.yml workflow with a workflow which for pull requests makes sure that users don't update action/index.js
  • Update action.yml to refer to action/index.js instead of dist/index.js

@Marcono1234
Copy link
Contributor

Have created a proof of concept here: #187

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.

None yet

2 participants