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.

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

Jump to bottom

Support VSO package manager registry #2507

Closed
wants to merge 5 commits into from
Closed

Support VSO package manager registry #2507

wants to merge 5 commits into from

Conversation

lyweiwei
Copy link

This is for a pre-review for the fix of #2505,
Will add test cases if the maintainer team agrees the basic idea.

Root cause

As described in #2505, for VSO package manager registries the package tarball's URL is not prefixed with the registry URL.

Fix

When downloading tarball, calculate the auth method with the original package nam instead of the tarball URL, if the URL is resolved from an NPM registry).

@lyweiwei lyweiwei changed the title Support VSO package manager registry Support VSO package manager registry (pre-review) Jan 19, 2017
@bestander
Copy link
Member

Tests are broken.
Also very much appreciated would be new tests added

@lyweiwei
Copy link
Author

@bestander I just pushed a fix for the failed test cases. But it seems some of the cases timeout due to network issues. Is this a know issue?

Summary of all failing tests
FAIL tests/commands/install/integration.js (205.516s)
● install incompatible optional dependency should still install shared child dependencies

Timeout - Async callback was not invoked within timeout specified by jasmine.DEFAULT_TIMEOUT_INTERVAL.

 at ontimeout (timers.js:365:14)
 at tryOnTimeout (timers.js:237:5)
 at Timer.listOnTimeout (timers.js:207:5)

FAIL tests/package-resolver.js (88.384s)
● resolve react-native

Timeout - Async callback was not invoked within timeout specified by jasmine.DEFAULT_TIMEOUT_INTERVAL.

 at ontimeout (timers.js:365:14)
 at tryOnTimeout (timers.js:237:5)
 at Timer.listOnTimeout (timers.js:207:5)

These test cases passed on Travis.CI

@lyweiwei
Copy link
Author

BTW, can we do a retry on the failed environments?

@bestander
Copy link
Member

@lyweiwei, yeah I think 3 retries could be reasonable.
Most of the network tests are actually mocked with checked in caches, so CI is not that unstable.
Probably your commit happened during some bad time.

@bestander
Copy link
Member

bestander commented Jan 25, 2017 •
edited
Loading

Yeah, I think the logic is reasonable.
Please go on with some tests and let's merge it once we are sure we don't break any previous scenarios

@bestander bestander self-requested a review January 25, 2017 22:22
@zumwald
Copy link

zumwald commented Feb 2, 2017

bump @bestander @lyweiwei what's the path forward for vsts support?

@bestander
Copy link
Member

@zumwald, this PR is quite good, it just needs a test and a rebase.
If you want to take it over, send a new PR with required changes and we'll merge it in.

@lyweiwei
Copy link
Author

lyweiwei commented Mar 2, 2017

I just pushed a new iteration

  • merge the latest changes from master
  • wait longer for the package-resolver test cases (6s->10s)

@lyweiwei
Copy link
Author

lyweiwei commented Mar 2, 2017

@bestander The PR passed the test this time :) sorry for the delay.

@lyweiwei lyweiwei changed the title Support VSO package manager registry (pre-review) Support VSO package manager registry Mar 2, 2017
bestander
bestander reviewed Mar 7, 2017
View reviewed changes
src/registries/is-request-to-registry.js
@@ -2,18 +2,25 @@

import url from 'url';

const SUFFIX_VISUALSTUDIO = '.pkgs.visualstudio.com';
Copy link
Member

Choose a reason for hiding this comment

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

I think I missed that detail earlier.
It looks hard to scale a maintain a solution where every registry has to be hardcoded in Yarn.
Can this setting be moved to .yarnrc for each registry to configure?

bestander
bestander reviewed Mar 7, 2017
View reviewed changes
__tests__/package-resolver.js
@@ -10,7 +10,7 @@ import * as fs from '../src/util/fs.js';
import * as constants from '../src/constants.js';
import inquirer from 'inquirer';

jasmine.DEFAULT_TIMEOUT_INTERVAL = 60000;
jasmine.DEFAULT_TIMEOUT_INTERVAL = 100000;
Copy link
Member

Choose a reason for hiding this comment

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

Does it mean that requests get slower after this change?

Copy link
Member

Choose a reason for hiding this comment

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

Also can you add a test here for the supported case?

mshustov
mshustov reviewed Mar 15, 2017
View reviewed changes
src/registries/npm-registry.js
const alwaysAuth = this.getScopedOption(registry.replace(/^https?:/, ''), 'always-auth')
|| this.getOption('always-auth')
|| removePrefix(requestUrl, registry)[0] === '@';
const alwaysAuth = this.getRegistryOrGlobalOption(registry, 'always-auth');
Copy link

@mshustov mshustov Mar 15, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@bestander does it make sense to force adding auth key to resoled url?
I faced similar problem some months ago with our artifactory.

@swese44 swese44 mentioned this pull request Apr 5, 2017
Closed
@kenotron
Copy link
Contributor

kenotron commented Apr 14, 2017 •
edited
Loading

@lyweiwei Can you remove the VSTS specific constant here and still make Yarn work against VSO branch? Or did you find a different work around?

@lumaxis lumaxis mentioned this pull request Apr 22, 2017
Merged
@lumaxis
Copy link
Contributor

lumaxis commented Apr 22, 2017

@lyweiwei @bestander I tried picking continuing this in #3231. Would very much appreciate a review! 🙂

bestander pushed a commit that referenced this pull request May 12, 2017
@lumaxis @bestander
[Replace #2507] Support VSTS and other custom registries with unusual…
3382071 
… feeds (#3231)

* Fix issue 2505

* Fix flow check

* Fix test errors

* Load custom package host suffix from .yarnrc

* Update tests with new customHostPrefix parameter

* Use customHostSuffix parameter to determine if request is to registry
@bestander
Copy link
Member

This one got replaced with #3231

@bestander bestander closed this May 12, 2017
@pensierinmusica pensierinmusica mentioned this pull request Jul 6, 2017
Closed
@lumaxis lumaxis mentioned this pull request Jan 8, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mshustov mshustov mshustov left review comments

@bestander bestander bestander left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@lyweiwei @bestander @zumwald @kenotron @lumaxis @mshustov