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.

Sign up for GitHub

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

Jump to bottom

build: ignore npm scripts when installing dependencies in dependent analyzer workflow #3013

Merged
merged 2 commits into from
Feb 28, 2024
Merged

build: ignore npm scripts when installing dependencies in dependent analyzer workflow #3013

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
861ff8c
build: ignore npm scripts when installing dependencies in dependent a…
viktorrusakov Feb 27, 2024
48ed494
chore: upgrade husky to v9
viktorrusakov Feb 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions .github/workflows/analyze-dependents.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@ name: Analyze Dependents

on:
workflow_dispatch:
# schedule:
# - cron: "0 14 * * *" # daily at 10am EST (Github Actions works with UTC)
schedule:
- cron: "0 14 * * *" # daily at 10am EST (Github Actions works with UTC)

jobs:
# clones known dependent Git repositories from Open edX
Expand Down Expand Up @@ -227,7 +227,7 @@ jobs:
with:
node-version: ${{ env.NODE_VER }}
- name: Install dependencies
run: npm ci
run: npm ci --ignore-scripts
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will skip running npm prepare which is run during npm ci by default.

Copy link
Member

@adamstankiewicz adamstankiewicz Feb 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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!)

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I'm not a big fan of husky as well haha, feels like it causes more trouble than helps us

Copy link
Member

Choose a reason for hiding this comment

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

Personally, I agree as well! I tend to use --no-verify out of habit in husky enabled repos lol.

working-directory: dependent-usage-analyzer
- name: Download dependent project checkouts
uses: actions/download-artifact@v2
Expand Down
Loading