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

Fix tarball file #3221

Merged
merged 3 commits into from
Apr 26, 2017
Merged

Fix tarball file #3221

merged 3 commits into from
Apr 26, 2017

Conversation

arcanis
Copy link
Member

@arcanis arcanis commented Apr 21, 2017

Summary

The offline mirror feature accidentally disabled the codepath that was used for installing packages from file: protocol. The tests didn't caught this error because they're using a wrapper class, LocalTarballFetcher, that wasn't affected by this issue.

@arcanis arcanis requested a review from bestander April 21, 2017 14:03
@arcanis
Copy link
Member Author

arcanis commented Apr 21, 2017

Wait, the revert shouldn't be there, I'll strip it

@arcanis
Copy link
Member Author

arcanis commented Apr 21, 2017

👍

@bestander
Copy link
Member

Looks like a few tests failed

@arcanis
Copy link
Member Author

arcanis commented Apr 25, 2017

Should be fine now

@@ -10,11 +10,16 @@ import {getPackageVersion, runInstall} from '../_helpers.js';
import {promisify} from '../../../src/util/promise';

jasmine.DEFAULT_TIMEOUT_INTERVAL = 150000;

test.concurrent = test.skip;
Copy link
Member

Choose a reason for hiding this comment

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

oops :)

Copy link
Member Author

Choose a reason for hiding this comment

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

😱

@bestander
Copy link
Member

@arcanis, you forgot to reenable tests

@bestander bestander merged commit d54fff3 into yarnpkg:master Apr 26, 2017
bestander pushed a commit that referenced this pull request May 2, 2017
* Fixes #3168

* Adds tests

* Fixes linting
@DanBuild DanBuild mentioned this pull request May 2, 2017
@bestander
Copy link
Member

This PR fixes relative paths but not absolute ones both on Windows and Unix.
Is it safe to add a root path to the regex, @arcanis?

@arcanis
Copy link
Member Author

arcanis commented May 3, 2017

I could change the regex to: /^(?=(?:\.{1,2}|[a-z]:)?[\\\/])/i, what do you think?

@arcanis
Copy link
Member Author

arcanis commented May 3, 2017

It seems to catch everything: https://regex101.com/r/zS5bmW/1

@bestander
Copy link
Member

Looks good to me, can you send a PR when you have a chance?

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.

None yet

2 participants