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

chore: replace husky with pre-commit ensure dist integrity #515

Closed
wants to merge 6 commits into from

Conversation

Uzlopak
Copy link
Contributor

@Uzlopak Uzlopak commented Oct 11, 2023

Checklist

@Uzlopak Uzlopak requested a review from simoneb October 11, 2023 10:55
@Uzlopak Uzlopak linked an issue Oct 11, 2023 that may be closed by this pull request
2 tasks
@Uzlopak
Copy link
Contributor Author

Uzlopak commented Oct 11, 2023

@simoneb

This should avoid the unintentional commit of the dist.

Copy link
Collaborator

@simoneb simoneb left a comment

Choose a reason for hiding this comment

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

Oh but we need the dist to be generated and committed when we do releases!

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Oct 11, 2023

@simoneb
I added now an integrity check for the dist file.

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Oct 11, 2023

As you can see the workflows for the commit 1a0ff43 failed because the dist file was not what we would expect. In e9fcfec the integrity check passes.

@Uzlopak Uzlopak changed the title chore: replace husky with pre-commit, disable commiting dist folder chore: replace husky with pre-commit ensure dist integrity Oct 11, 2023
@simoneb
Copy link
Collaborator

simoneb commented Oct 12, 2023

Apologies but I don't see the value in this solution. I like the simplicity of building the action automatically when committing, so that dist is always aligned with the source code.

The only downside to this approach is that sometimes dist is changed in PRs that are not changing the source code, and the most likely reason is that some packages got updated in the PR author's branch. I don't find this a major issue since the dist will nonetheless be recreated when we release the action by the very same process.

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Oct 12, 2023

@simoneb

I understand. I use this type of logic in the actions at my employer. See e.g.
https://github.com/metro-digital/setup-tools-for-waas

So it results in two things:

  1. If a contributor pushes changes, and also changes the dist files, we can trust the modified dist. When @groozin was pushing we asked him to revert that file. But with the integrity check, we could have accepted it without worrying.

  2. When we get some PR by dependabot, which does change the dist file, it gets detected and automerge is prevented. We can run build.yml workflow on that dependabot branch and a correct dist is generated. We must then only squash merge manually. Benefit of this is, that we can actually see, if there was relevant changed and make a release. So e.g. if dependabot updates eslint, it will be merged silently. But if it changes @actions/core we see that a PR got not merged automatically and we can see that it effected the dist file. So we should consider a new release.

  3. Also we could rebuild the dist by running build.yml targetting main.

@Uzlopak Uzlopak closed this Apr 10, 2024
@Uzlopak Uzlopak deleted the replace-husky-with-pre-commit branch April 10, 2024 18:49
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.

replace husky with @fastify/pre-commit
2 participants