build: ignore npm scripts when installing dependencies in dependent analyzer workflow #3013
Conversation
|
Thanks for the pull request, @viktorrusakov! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
openedx-webhooks
added
the
open-source-contribution
✅ Deploy Preview for paragon-openedx ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
| @@ -227,7 +227,7 @@ jobs: | |||
| with: | |||
| node-version: ${{ env.NODE_VER }} | |||
| - name: Install dependencies | |||
| run: npm ci | |||
| run: npm ci --ignore-scripts | |||
There was a problem hiding this comment.
This will skip running npm prepare which is run during npm ci by default.
There was a problem hiding this comment.
The proposed fix here makes sense to me, though I'm wondering if you also considered exploring the recommendation in the husky documentation here:
If installing only dependencies (not devDependencies), the "prepare": "husky" script may fail because Husky won't be installed. Modify the prepare script to never fail:
// package.json
"prepare": "husky || true"
Similar sanity-check type question: the prepare script in package.json includes husky install, which seems to differ a bit from just the husky command from their docs above. I do see we have v8 of husky installed when v9 is latest so maybe this bit is due to the version discrepancy? 🤔
There was a problem hiding this comment.
Hmm, I haven't seen that part in their documentation, let me try that as well. And I didn't notice we are behind the latest version as well 😅 . (Thanks for the quick review!)
There was a problem hiding this comment.
I personally would like to see us just completely remove husky. I'm not a fan of git hooks - I find they get in the way when I just want to push something WIP up real quick (I know I can use git push --no-verify but still), and in general they slow down pushes. I also see them as redundant in our case considering the checks we have in CI.
That's definitely a conversation that is out of scope for this PR though, so if husky || true works I say 
There was a problem hiding this comment.
OK, it worked, I updated husky to v9 as well (the steps to do that were pretty easy). It does log a small error husky: command not found now whenever you try to install dependencies in dependent-usage-analyzer directory, but I think it should be ok (they have a fix for that as well in docs, but it only works in CI environments, e.g. github actions. And it crashes if you install deps locally, but that's because we use workspaces I think...)
There was a problem hiding this comment.
Yeah, I'm not a big fan of husky as well haha, feels like it causes more trouble than helps us
There was a problem hiding this comment.
Personally, I agree as well! I tend to use --no-verify out of habit in husky enabled repos lol.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3013 +/- ##
=======================================
Coverage 93.18% 93.18%
=======================================
Files 249 249
Lines 4342 4342
Branches 1036 1036
=======================================
Hits 4046 4046
Misses 292 292
Partials 4 4 ☔ View full report in Codecov by Sentry. |
|
@adamstankiewicz @brian-smith-tcril can you please review this? It fixes the issue locally for me. |
|
@viktorrusakov 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
|
🎉 This PR is included in version 22.2.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Description
Fixes #3000 which was caused by adding
preparelife cycle script to the root package.json file which installs husky. It works fine if you run the script in the root directory but it fails if you run it from somewhere else (e.g.dependent-usage-analyzerdirectory from whichnpm ciis run during the workflow which I assume triggersnpm run prepareof root package.json because we use workspaces). The reason is that husky is configured to be run from the root directory.Merge Checklist
exampleapp?wittjeffandadamstankiewiczas reviewers on this PR.Post-merge Checklist