Skip to content

Add PR check to remove npm absolute paths #29

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

Merged
robertbrignull merged 3 commits into from
May 13, 2020
Merged

Add PR check to remove npm absolute paths #29

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
256c63a
Add and run removeNPMAbsolutePaths
robertbrignull May 12, 2020
ec4d38a
add PR check of node modules
robertbrignull May 12, 2020
51b42fc
Merge branch 'master' into removeNPMAbsolutePaths
robertbrignull May 12, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
The table of contents is too big for display.
Diff view
Diff view
  •  
  •  
  •  
30 changes: 29 additions & 1 deletion .github/workflows/lint-build-test.yml → .github/workflows/pr-checks.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
name: "Lint, Build & Test"
name: "PR checks"

on: [push, pull_request]

Expand Down Expand Up @@ -30,10 +30,38 @@ jobs:
if [ ! -z "$(git status --porcelain)" ]; then
# If we get a fail here then the PR needs attention
>&2 echo "Failed: JavaScript files are not up to date. Run 'npm run-script build' to update"
git status
exit 1
fi
echo "Success: JavaScript files are up to date"

check-node-modules:
runs-on: ubuntu-latest

steps:
- uses: actions/checkout@v1
- name: Check node modules up to date
run: |
# Sanity check that repo is clean to start with
if [ ! -z "$(git status --porcelain)" ]; then
# If we get a fail here then this workflow needs attention...
>&2 echo "Failed: Repo should be clean before testing!"
exit 1
fi

# Reinstall modules and then clean to remove absolute paths
# Use 'npm ci' instead of 'npm install' as this is intended to be reproducible
npm ci
npm run removeNPMAbsolutePaths
Copy link
Contributor

@Daverlo Daverlo May 12, 2020

Choose a reason for hiding this comment

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

Should this have the --force flag?

The reason I added the --force flag to removeNPMAbsolutePaths is so that it rewrites all files to a canonical form. I was seeing some differences in ordering of properties of json objects when there were otherwise no changes. I hope by letting removeNPMAbsolutePaths decide this aspect we'll avoid any inconsistencies.

Copy link
Contributor Author

@robertbrignull robertbrignull May 12, 2020

Choose a reason for hiding this comment

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

No, that's a reference to the script defined in package.json

codeql-action/package.json

Line 10 in 51b42fc

"removeNPMAbsolutePaths": "removeNPMAbsolutePaths . --force"

I did it that way so then we can keep things consistent by only specifying the arguments there.

It is perhaps a bit confusing that the name is the same, unless you know what is going on. What do you think?

Copy link
Contributor

@Daverlo Daverlo May 12, 2020

Choose a reason for hiding this comment

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

I missed that. If it works I'm happy with it

# Check that repo is still clean
if [ ! -z "$(git status --porcelain)" ]; then
# If we get a fail here then the PR needs attention
>&2 echo "Failed: node_modules are not up to date. Run 'npm ci' and 'npm run removeNPMAbsolutePaths' to update"
git status
exit 1
fi
echo "Success: node_modules are up to date"

npm-test:
runs-on: ubuntu-latest

Expand Down
1 change: 1 addition & 0 deletions node_modules/.bin/removeNPMAbsolutePaths
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 7 additions & 1 deletion node_modules/@actions/http-client/README.md
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

16 changes: 16 additions & 0 deletions node_modules/@actions/http-client/RELEASES.md
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 5 additions & 2 deletions node_modules/@actions/http-client/auth.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

24 changes: 14 additions & 10 deletions node_modules/@actions/http-client/index.d.ts
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading