Skip to content

fix: added check for dry-run #7274

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
merged 2 commits into from
Mar 13, 2024
Merged

fix: added check for dry-run #7274

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4d224ee
fix: added check for dry-run
cod1r Mar 9, 2024
3a7a20a
chore: added test for ci --dry-run check
cod1r Mar 10, 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
18 changes: 10 additions & 8 deletions lib/commands/ci.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,14 +75,16 @@ class CI extends ArboristWorkspaceCmd {
)
}

// Only remove node_modules after we've successfully loaded the virtual
// tree and validated the lockfile
await this.npm.time('npm-ci:rm', async () => {
const path = `${where}/node_modules`
// get the list of entries so we can skip the glob for performance
const entries = await fs.readdir(path, null).catch(er => [])
return Promise.all(entries.map(f => fs.rm(`${path}/${f}`, { force: true, recursive: true })))
})
if (!this.npm.flatOptions.dryRun) {
// Only remove node_modules after we've successfully loaded the virtual
// tree and validated the lockfile
await this.npm.time('npm-ci:rm', async () => {
const path = `${where}/node_modules`
// get the list of entries so we can skip the glob for performance
const entries = await fs.readdir(path, null).catch(er => [])
return Promise.all(entries.map(f => fs.rm(`${path}/${f}`, { force: true, recursive: true })))
})
}
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe it'd be better if, when a dry run, it checked that npm has permissions to rimraf node_modules?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure what you mean by 'permissions'. Sorry this is my first time contributing to npm.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It wasn't checking for any permissions before so I am confused.

Copy link
Contributor

Choose a reason for hiding this comment

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

You’re right, but i meant it’d be useful and match the “dry run” semantics

Copy link
Member

Choose a reason for hiding this comment

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

Checking permissions in node is mostly done by trying the operation and then reporting to the user if it failed. It's even discouraged for things like fs.access to be used for this purpose. With that in mind I think it's fine for dry-run to simply tell the user what it would try to do. It is not meant to be a test of if npm will eventually be able to succeed. Many other things could go wrong (i.e. running out of disk space).

This PR appears fine as-is (I am not done fully reviewing it but at a glance it appears mostly on the right track).

Copy link
Contributor

Choose a reason for hiding this comment

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

presumably deleting files wouldn't cause disk space to run out :-)

def that enhancement could be done as a follow through regardless


await arb.reify(opts)

Expand Down