fix(fetcher): offline mirror name collision w/ private registries and scopes #4822

newren · 2017-11-01T17:06:42Z

Summary

When using artifactory as an internal npm registry in conjunction with scoped packages, yarn install was failing (about 50% of the time) with messages of the form

  Hashes don't match when extracting file
  "https://artifactory.internal.site:443/artifactory/api/npm/external-mirror/@types/react/-/react-15.6.4.tgz".
  Expected "3bb57bd43183a05919ceb025a264287348f47e9d" but got
  "da39a3ee5e6b4b0d3255bfef95601890afd80709"

The problem was that yarn was writing both @types/react-15.6.4.tgz and react-15.6.4.tgz to the same location within the offline mirror due to making an assumption about no additional leading paths before the scope name in the URL -- an assumption which was true for registry.npmjs.org, but not for artifactory npm repositories.

Test plan

Added a new unit test.

buildsize · 2017-11-01T17:12:31Z

This change will decrease the build size from 10.24 MB to 9.94 MB, a decrease of 301.14 KB (3%)

File name	Previous Size	New Size	Change
yarn-[version].noarch.rpm	885.44 KB	860.16 KB	-25.28 KB (3%)
yarn-[version].js	3.85 MB	3.78 MB	-70.2 KB (2%)
yarn-legacy-[version].js	4 MB	3.84 MB	-168.8 KB (4%)
yarn-v[version].tar.gz	890.08 KB	865.72 KB	-24.36 KB (3%)
yarn_[version]all.deb	667.31 KB	654.81 KB	-12.5 KB (2%)

newren · 2017-11-01T21:05:17Z

The two failures in one of the travis builds look to me like unrelated test flakes; the code I touched shouldn't affect either of those two operations. Is there a way to retrigger the build to see if it duplicates?

BYK

Thanks a lot for doing this!

I have 3 requests before merging tho:

A more concise and accurate PR summary. Right now I understand the problem but I cannot determine if it is the offline mirror or just the cache. I'm guessing it is the cache.
Naming those constants you have changed. What is 4 and why is it 4? Why does it mean scoped? These are not obvious from the code.
At least one test that was failing without your fix and passing with the fix.

Sounds good?

… packages When using artifactory as an internal npm registry in conjunction with scoped packages, yarn install was failing (about 50% of the time) with messages of the form Hashes don't match when extracting file "https://artifactory.internal.site:443/artifactory/api/npm/external-mirror/@types/react/-/react-15.6.4.tgz". Expected "3bb57bd43183a05919ceb025a264287348f47e9d" but got "da39a3ee5e6b4b0d3255bfef95601890afd80709" The problem was that yarn was writing both @types/react-15.6.4.tgz and react-15.6.4.tgz to the same location within the offline mirror due to making an assumption about no additional leading paths before the scope name in the URL -- an assumption which was true for registry.npmjs.org, but not for artifactory npm repositories.

newren · 2017-11-02T21:16:20Z

Thanks for the review! I've tried to address each of your three issues, but I'm still a newbie to the web development side of programming, so if I missed anything or there's further changes you'd like to see, just let me know.

newren · 2017-11-03T16:17:34Z

The failed builds look like flakes to me; my code didn't touch the codepaths of the tests that failed, plus some were just timeouts. Is there a way to retrigger those tests or something?

newren · 2017-11-14T22:01:09Z

Hey, @BYK was just curious if my changes still needed further fixes or if they're now to your liking. (As mentioned above, the test failures look like unrelated flakes and I don't seem to have perms to retrigger the builds.) If you're just busy, I understand; just pinging in case my update fell through the cracks.

BYK · 2017-12-06T16:41:27Z

Woops, sorry for the silence. This fell off my radar. Doing another pass now.

@arcanis

I changed the code a bit so need @arcanis to review

newren

Thanks for circling back! It's good to see this moving forward again. We're internally using a fork of yarn, but really don't want to do that forever. Your changes look like they'd handle most cases, but I think there's an issue with internal team-scoped packages.

newren · 2017-12-06T20:46:25Z

src/fetchers/tarball-fetcher.js

@@ -17,6 +17,8 @@ const stream = require('stream');
 const gunzip = require('gunzip-maybe');
 const invariant = require('invariant');

+const RE_URL_NAME_MATCH = /\/(?:(@[^/]+)\/)?[^/]+\/-\/([^/]+)$/;


I think this will fail for some packages. For example, looking at some sample resolved urls in a random local yarn.lock, it appears that most would match your regex, such as ones of the form

https://publish.internal.site:443/artifactory/api/npm/external-npm/abab/-/abab-1.0.3.tgz
or
https://publish.internal.site:443/artifactory/api/npm/external-npm/@types/react/-/react-15.0.38.tgz

However, there are also some of the form

https://artifactory.internal.site:443/artifactory/api/npm/internal-npm-release/@team/package/-/@team/package-1.2.3.tgz

It looks like all internally team-scoped packages appear to have an extra "/@team/" between the "/-/" and the tarballBasename.

That's quite weird and it was also what I feared. We can tune the regex to handle these too but ideally, we should not be extracting the scope information from the URL anyway.

Since changing the names of these files would invalidate any existing offline mirror, we need to find a migration path when doing this in a better way.

I've pushed a new change but I also need to extend and refactor the tests to cover this new scenario. Feel free to add them since I can't look at this patch until tomorrow.

…readdir_files * upstream/master: fix(cli): Write Node4+ error message to stderr (yarnpkg#5094) test(jest): Upgrade jest to latest available version (yarnpkg#5018) fix(git): Ignores irrelevant output from ls-remote (yarnpkg#5081) test(integration): Fix failing react-scripts test due to unexpected warning (yarnpkg#5076) feat(help) Add command descriptions to commander output (yarnpkg#5033) ci(circle): Fix cache key setup for proper node_modules sharing (yarnpkg#5060) fixed (yarnpkg#5034) fix(fetcher): offline mirror name collision w/ private registries and scopes (yarnpkg#4822) fix(add): Make semver flags compatible with versioned requests (yarnpkg#4999) [yarnpkg#5021] Add help comment to --json flag (yarnpkg#5045) fix(git): match git dependencies by name instead of whole url fix(install): connectionOptions passes in localhost as its host to prevent popup on MacOsx. (yarnpkg#5006) chore(eslint): ignore packages dir (yarnpkg#4963)

newren mentioned this pull request Nov 1, 2017

Fixes scoped packages handling with offline installation #4815

Closed

newren mentioned this pull request Nov 1, 2017

Issue with offline installation of @types/node #4266

Open

BYK previously requested changes Nov 2, 2017

View reviewed changes

newren force-pushed the fix/yarn-local-mirror-scoping-issue branch from d7111f6 to b85ab09 Compare November 2, 2017 21:03

newren changed the title ~~Fix offline mode with scoped packages, particularly for typescript~~ Fix offline mirror file collision with internal registries and scoped packages Nov 2, 2017

BYK added 2 commits December 6, 2017 16:43

Merge branch 'master' into fix/yarn-local-mirror-scoping-issue

b51d2cc

Use regex for stronger guarantees

2ecb459

BYK requested a review from arcanis December 6, 2017 17:29

BYK changed the title ~~Fix offline mirror file collision with internal registries and scoped packages~~ fix(fetcher): Fix offline mirror file collision with internal registries and scoped packages Dec 6, 2017

BYK changed the title ~~fix(fetcher): Fix offline mirror file collision with internal registries and scoped packages~~ fix(fetcher): offline mirror name collision w/ private registries and scopes Dec 6, 2017

Move '@' into the scope name

61c4ec1

newren commented Dec 6, 2017

View reviewed changes

BYK added 2 commits December 6, 2017 23:14

extend the regex

f1d3bd2

Add fallback mechanism when regex fails

a0886e9

arcanis approved these changes Dec 7, 2017

View reviewed changes

BYK added 2 commits December 7, 2017 11:39

prefer match over exec

3dfa3e2

Add another test case

93b6545

BYK merged commit ec8cea0 into yarnpkg:master Dec 7, 2017

KevinGrandon mentioned this pull request Jun 11, 2019

yarn-offline-mirror scoped packages regression 1.3.2 --> 1.5.1 #5634

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(fetcher): offline mirror name collision w/ private registries and scopes #4822

fix(fetcher): offline mirror name collision w/ private registries and scopes #4822

newren commented Nov 1, 2017 •

edited by BYK

Loading

buildsize bot commented Nov 1, 2017 •

edited

Loading

newren commented Nov 1, 2017

BYK left a comment

newren commented Nov 2, 2017

newren commented Nov 3, 2017

newren commented Nov 14, 2017

BYK commented Dec 6, 2017

newren left a comment

newren Dec 6, 2017

BYK Dec 6, 2017

BYK Dec 6, 2017

fix(fetcher): offline mirror name collision w/ private registries and scopes #4822

fix(fetcher): offline mirror name collision w/ private registries and scopes #4822

Conversation

newren commented Nov 1, 2017 • edited by BYK Loading

buildsize bot commented Nov 1, 2017 • edited Loading

newren commented Nov 1, 2017

BYK left a comment

Choose a reason for hiding this comment

newren commented Nov 2, 2017

newren commented Nov 3, 2017

newren commented Nov 14, 2017

BYK commented Dec 6, 2017

newren left a comment

Choose a reason for hiding this comment

newren Dec 6, 2017

Choose a reason for hiding this comment

BYK Dec 6, 2017

Choose a reason for hiding this comment

BYK Dec 6, 2017

Choose a reason for hiding this comment

newren commented Nov 1, 2017 •

edited by BYK

Loading

buildsize bot commented Nov 1, 2017 •

edited

Loading