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 yarn registry with command argument #7109

Merged
merged 8 commits into from Sep 20, 2019
Merged

Conversation

@fcollonval
Copy link
Member

@fcollonval fcollonval commented Aug 28, 2019

References

Fixes #3658

Code changes

By default JupyterLab reads the NPM package repository from the yarn config. The purpose is to overwrite the default yarn repository in the yarn.lock file shipped with JLab for the 3 following cases:

  • Build the application through: jupyter lab build
  • Installing an application through: jupyter labextension install
  • Launching the application in watch mode through: jupyter lab --watch

User-facing changes

None

Backwards-incompatible changes

None

@jupyterlab-dev-mode
Copy link

@jupyterlab-dev-mode jupyterlab-dev-mode bot commented Aug 28, 2019

Thanks for making a pull request to JupyterLab!

To try out this branch on binder, follow this link: Binder

@fcollonval
Copy link
Member Author

@fcollonval fcollonval commented Aug 28, 2019

Currently to work with the configuration file jupyter_config.json, three identical parameters must be specified:

{
  "LabApp":{
    "yarn_registry": "https://artifactory.server:8080/artifactory/api/npm/npm-all/"
  },
  "BaseExtensionApp":{
    "yarn_registry": "https://artifactory.server:8080/artifactory/api/npm/npm-all/"
  },
  "LabBuildApp":{
    "yarn_registry": "https://artifactory.server:8080/artifactory/api/npm/npm-all/"
  }
}

I'ld like to create a new class LabCommonApp(SingletonConfigurable) that would gather the common parameters between LabApp, BaseExtensionApp and LabBuildApp - currently it will be app_dir and yarn_registry. This will allow to specify only one parameter in the configuration file.

Another idea would be to get the registry from yarn config list --json and apply the patch if the registry is not the default. But it looks there are no use of stdout process capturing in any place (except for debug). I don't know if this is desired or is unneeded.

@blink1073 @jasongrout What do you think?

@blink1073
Copy link
Member

@blink1073 blink1073 commented Aug 28, 2019

We use stdout capture for the node version. I like that solution for the yarn registry.

@fcollonval
Copy link
Member Author

@fcollonval fcollonval commented Aug 28, 2019

Thx for the tip. I tested at work tomorrow and add some unit tests.

@fcollonval
Copy link
Member Author

@fcollonval fcollonval commented Aug 30, 2019

It is ready for review.

@vidartf I overwrite the registry attribute of _AppHandler with the yarn registry from the yarn config (only for build, watch and extension installation). I haven't seen any bugs when doing so. But could you confirm it's safe as by default it is set to npm registry not yarn?

@fcollonval fcollonval marked this pull request as ready for review Aug 30, 2019
Copy link
Member

@blink1073 blink1073 left a comment

This LGTM, leaving open for @vidartf's sign-off

Copy link
Member

@vidartf vidartf left a comment

I agree with the principle. Some comments on implementation details below. Also, I think this could be cleaner if we wait on #7079 and put registry into the options object (I'm trying to fix that PR now).

jupyterlab/commands.py Show resolved Hide resolved
jupyterlab/tests/test_commands.py Outdated Show resolved Hide resolved
jupyterlab/commands.py Outdated Show resolved Hide resolved
@blink1073
Copy link
Member

@blink1073 blink1073 commented Sep 9, 2019

Thanks for your patience @fcollonval. Can you please rebase and take advantage of the new AppOptions from #7079 for this setting?

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Sep 9, 2019

Does this supersede #6495 and fix #4149?

@vidartf
Copy link
Member

@vidartf vidartf commented Sep 11, 2019

jupyterlab/commands.py Outdated Show resolved Hide resolved
jupyterlab/commands.py Show resolved Hide resolved
jupyterlab/commands.py Outdated Show resolved Hide resolved
@fcollonval
Copy link
Member Author

@fcollonval fcollonval commented Sep 20, 2019

@vidartf The error is unrelated. I think we can merge. Thx for the patience and discussion.

@fcollonval fcollonval merged commit 9aa4205 into jupyterlab:master Sep 20, 2019
9 checks passed
@fcollonval fcollonval deleted the fix-3658 branch Sep 20, 2019
shutil.copy(pjoin(HERE, 'staging', 'yarn.lock'), lock_path)
lock_path = pjoin(staging, 'yarn.lock')
lock_template = pjoin(HERE, 'staging', 'yarn.lock')
if self.registry != YARN_DEFAULT_REGISTRY: # Replace on the fly the yarn repository see #3658
Copy link
Member

@vidartf vidartf Sep 20, 2019

Choose a reason for hiding this comment

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

Note, that if the registry has been set to something non-default, and then been set back to default, a jupyter lab clean && jupyter lab build command is needed to reset. Ideally this should be documented.

@telamonian
Copy link
Member

@telamonian telamonian commented Oct 15, 2019

@meeseeksdev backport to 1.x

@programmer94
Copy link

@programmer94 programmer94 commented Nov 4, 2019

Does this solution allow for authentication to a private repository? Would we pass our credentials in the URL, i.e., https://[user]:[pwd]@artifactory.server:8080/artifactory/api/npm/npm-all
If not, how would one go about doing it?

@lock lock bot locked as resolved and limited conversation to collaborators Dec 4, 2019
@jasongrout jasongrout added this to the 2.0 milestone Feb 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

7 participants