ci: notify non generated dist on PR #63
Conversation
|
If we do it on push to main we can use the default GITHUB_TOKEN. |
Doesn't this require you to have permission on the default branch + no push restriction on the branch? I haven't set this up on public repos. |
Yes you're right when the branch is protected. |
.github/workflows/ci.yml
Outdated
| @@ -12,6 +12,7 @@ jobs: | |||
| - run: npm run lint | |||
| - run: npm run test | |||
| - name: Detect Generation needed Dist | |||
| if: github.actor != 'dependabot' | |||
There was a problem hiding this comment.
are you sure this is the name the actor gets when it runs as dependabot? I can think of more exceptions though. not all files end up generating a different dist folder contents. say changes to readme or anything else that is not source code really. are we sure it's safe to break the build just for the sake of somebody not running husky hooks properly? I'm not convinced honestly.
There was a problem hiding this comment.
I am +1 for simoneb stance. In addition, as a github action release need manually change the package.json version. It is not a good idea to force updating dist file without the change of package version.
There was a problem hiding this comment.
for example hooks do not run on my env (some projects running in WSL makes it crash), it's more convinient to ensure this in the CI. See: #61 (review)
Regarding dependabot AFAIK this is its account name.
Changes not in source code won't change the build artifact. On the other hand, dependabot will not run the build command and in the case of a dependency change we'll have to run it on our own.
The easiest thing is to run this on commit to default but like explain it can be a pain. Otherwise we can ship a Dockerfile which would be consumed with the build inside of it, but for our case it's kind of overkill.
There was a problem hiding this comment.
I am +1 for simoneb stance. In addition, as a
github actionrelease need manually change thepackage.jsonversion. It is not a good idea to force updatingdistfile without the change of package version.
A lot of people rely a non tagged actions, which means checking out the default branch. So in this case it means you're adding commits to the history without updating the code that the action will run. In the end the artifact of the default branch will not be in sync with the code.
There was a problem hiding this comment.
I'm sorry but I'm not in favor of this change. I'm all for automation when it removes work, but I'm not when it increases the amount of work you have to do.
- Without this automation we need to make sure that the dist folder is generated, which clearly can be a source of errors but it won't for most people because
- husky is a fairly standard tool and most setups will run it correctly
- missing dist/ changes will hopefully be caught by PR reviews
- With this automation some commits will start failing for no good reasons, and this to me is more maintenance burden than the first point
|
Note that part of the issue might be that the husky script was not executable #61 (comment) |
Add verification step for: #62
We can setup an automatic generation on pushing to the default branch, but for this we need a Personnal Access Token to be created to allow to push on the repo directly from CI. To be honest i prefer adding a verification step to ensure collaborators to run
npm run buildeven if there is ahuskyhookChecklist
npm run testandnpm run benchmarkand the Code of conduct