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

Don't compute integrity when network restricted #6248

Merged
merged 3 commits into from
Aug 10, 2018
Merged

Conversation

arcanis
Copy link
Member

@arcanis arcanis commented Aug 8, 2018

It seems that git repositories have no integrity in the lockfile. This causes issues with --offline, since the current try will try to regenerate it (even if it won't succeed), and crash because it cannot access the network.

There's two issues there:

  • Git repositories have no integrity. Even if we allow it, it should be marked in the lockfile (for example by setting null), in order to let Yarn know that the integrity couldn't be computed at all.

  • When working with --offline, we shouldn't try to generate the integrity if it doesn't exist. This is what this PR fixes.

cc @imsnif who worked on the integrity field.

@arcanis arcanis requested a review from imsnif August 8, 2018 16:34
@imsnif
Copy link
Member

imsnif commented Aug 9, 2018

@arcanis - is it possible to see a test case (or some repro) that this PR fixes?

@arcanis
Copy link
Member Author

arcanis commented Aug 9, 2018

@imsnif It can't be easily tested since it involves git repositories 😞

Basically this is the repro:

$> echo yarn-offline-mirror ./offline-mirror > .yarnrc

$> yarn add yarn@[email protected]:yarnpkg/yarn
# The sha hash is missing from the lockfile

$> yarn cache clean && rm -rf node_modules

$> yarn --offline
# Since the sha hash is missing, Yarn was trying to regenerate it and failed

@imsnif
Copy link
Member

imsnif commented Aug 10, 2018

So first - good catch! Turns out this is actually not unique to github deps, but to all deps that do not have an integrity field in yarn.lock (eg. when updating from an old yarn version).

Because of this, adding a test was possible. I added one that checks this basic scenario and fails if we revert your change. Wdyt @arcanis?

@arcanis
Copy link
Member Author

arcanis commented Aug 10, 2018

Oh nice! Looks great, thanks 👍

@arcanis arcanis merged commit b813ebe into master Aug 10, 2018
@arcanis arcanis deleted the arcanis-patch-1 branch August 13, 2018 20:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants