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

Revert loosening of npm audit strictness #1317

Closed
atmgrifter00 opened this issue Jun 22, 2023 · 0 comments · Fixed by #1322
Closed

Revert loosening of npm audit strictness #1317

atmgrifter00 opened this issue Jun 22, 2023 · 0 comments · Fixed by #1322
Assignees

Comments

@atmgrifter00
Copy link
Contributor

atmgrifter00 commented Jun 22, 2023

🧹 Tech Debt

The npm audit step of our pipeline was failing due to a vulnerability with semver. We made it less strict in #1318 to get the build passing, but we should fix the vulnerability and make it strict again instead.

@atmgrifter00 atmgrifter00 added tech debt triage New issue that needs to be reviewed labels Jun 22, 2023
@atmgrifter00 atmgrifter00 removed the triage New issue that needs to be reviewed label Jun 22, 2023
@atmgrifter00 atmgrifter00 self-assigned this Jun 22, 2023
@jattasNI jattasNI mentioned this issue Jun 22, 2023
1 task
@atmgrifter00 atmgrifter00 mentioned this issue Jun 22, 2023
1 task
@atmgrifter00 atmgrifter00 linked a pull request Jun 22, 2023 that will close this issue
1 task
rajsite pushed a commit that referenced this issue Jun 22, 2023
# Pull Request

## 🤨 Rationale

Workaround for #1317. We'll use that issue to track re-enabling strict
auditing.

## 👩‍💻 Implementation

The current error is a `moderate` vulnerability so changing the level to
`high` should temporarily allow it

## 🧪 Testing

Pipeline

## ✅ Checklist

<!--- Review the list and put an x in the boxes that apply or ~~strike
through~~ around items that don't (along with an explanation). -->

- [ ] I have updated the project documentation to reflect my changes or
determined no changes are needed.
@jattasNI jattasNI changed the title npm audit is currently failing builds Revert loosening of npm audit strictness Jun 22, 2023
@jattasNI jattasNI added the triage New issue that needs to be reviewed label Jun 22, 2023
jattasNI added a commit that referenced this issue Jun 23, 2023
…enable audit for prod (#1322)

# Pull Request

## 🤨 Rationale

Fixes #1317.

The `npm audit` of production dependencies was failing because of deps
that Storybook brought in. But Storybook should really be a dev
dependency.

Storybook is listed as a production dependency because it's in
`peerDependencies` in the root `package.json`. We added it there in
[this
commit](321cdd3)
of the PR that migrated us to Storybook 7. I believe the rationale was
that it was necessary to apply [a
patch](https://github.com/ni/nimble/blob/main/patches/%40storybook%2Bhtml%2B7.0.0.patch)
to the package. (I'd like to remove that patch but I think it's not
possible until storybookjs/storybook#22384 is
available. Currently it's only released in an alpha branch)

## 👩‍💻 Implementation

1. Remove `@storybook/html` from `peerDependencies` in the root
`package.json`
2. `git clean -fdx`
3. `npm install` to regenerate `package-lock.json`
4. Re-enable audit for all severity levels for prod dependencies

## 🧪 Testing

I locally verified that the patch was still applied to the file inside
node_modules. If it isn't applied, I believe we'd see a build error.

I locally verified that `npm audit --only=prod` succeeds now.

Otherwise relying on the PR build.

## ✅ Checklist

<!--- Review the list and put an x in the boxes that apply or ~~strike
through~~ around items that don't (along with an explanation). -->

- [ ] I have updated the project documentation to reflect my changes or
determined no changes are needed.
@rajsite rajsite removed the triage New issue that needs to be reviewed label Mar 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
3 participants