[GitHub] zeppelin pull request #2349: [ZEPPELIN-2214] Set npm installer default npm r...

classic Classic list List threaded Threaded
21 messages Options
12
Reply | Threaded
Open this post in threaded view
|

[GitHub] zeppelin pull request #2349: [ZEPPELIN-2214] Set npm installer default npm r...

zjffdu
GitHub user necosta opened a pull request:

    https://github.com/apache/zeppelin/pull/2349

    [ZEPPELIN-2214] Set npm installer default npm registry

    ### What is this PR for?
    Complements ZEPPELIN-2278, this is a fix for "connection timeout" bug when trying to run "npm install" behind a proxy. Instead of connecting to the official nodejs site from behind a proxy, we should use the already soft-coded env variable ZEPPELIN_HELIUM_NPM_REGISTRY repository.
   
    ### What type of PR is it?
    Bug Fix
   
    ### ToDos
    * [ ] - Soft-code npm folder name as well (currently hardcoded "npm") - To be agreed on...
   
    ### What is the Jira issue?
    * [ZEPPELIN-2214 - Bug on connecting to the official nodejs site from behind a proxy]
   
    ### How should this be tested?
    Enable any Helium plugin from behind a proxy
   
    ### Questions:
    * Does the licenses files need update? No
    * Is there breaking changes for older versions? No
    * Does this needs documentation? No


You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/nokia/zeppelin zeppelin2214

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/zeppelin/pull/2349.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #2349
   
----
commit 3f453c294dba8a746ef273f56e3ffb0bb8c37c2c
Author: Nelson Costa <[hidden email]>
Date:   2017-05-17T14:12:58Z

    [ZEPPELIN-2214] Set npm installer default npm registry

----


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] zeppelin issue #2349: [ZEPPELIN-2214] Set npm installer default npm registry

zjffdu
Github user andreaTP commented on the issue:

    https://github.com/apache/zeppelin/pull/2349
 
    Since npm registry name can be changed arbitrarily it's worth to add a configuration key for the hard-coded `"/npm/-/"`


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] zeppelin issue #2349: [ZEPPELIN-2214] Set npm installer default npm registry

zjffdu
In reply to this post by zjffdu
Github user necosta commented on the issue:

    https://github.com/apache/zeppelin/pull/2349
 
    Travis is happy, Jenkins is unhappy. Anyone knows why?
    ```
    python ./travis_check.py
    Traceback (most recent call last):
      File "./travis_check.py", line 35, in <module>
        author = sys.argv[1]
    IndexError: list index out of range]
    ```


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] zeppelin issue #2349: [ZEPPELIN-2214] Set npm installer default npm registry

zjffdu
In reply to this post by zjffdu
Github user 1ambda commented on the issue:

    https://github.com/apache/zeppelin/pull/2349
 
    @necosta
   
    What is your fork name? It should be `zeppelin`.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] zeppelin issue #2349: [ZEPPELIN-2214] Set npm installer default npm registry

zjffdu
In reply to this post by zjffdu
Github user necosta commented on the issue:

    https://github.com/apache/zeppelin/pull/2349
 
    Thanks @andreaTP , @1ambda , @felixcheung
   
    @andreaTP , I confused everyone with soft-coding the npm value. "npm" is the npm package name. So it should remain hard-coded.
   
    @1ambda , thanks for your help. It's my first pull request so might have some issues with the CI. Answering your question, I believe I forked from zeppelin as I'm making the pull request from nokia (remote) / zeppelin (repo) / zeppelin2214 (branch). Let me know if I'm still missing something.
   
    @felixcheung , @1ambda , ignore Npm registry repo. Latest version has now soft-coded Node registry, NPM registry and YARN registry. Node and Yarn are repositories, not registries, but with a repository manager, they become registry paths.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] zeppelin issue #2349: [ZEPPELIN-2214] Set npm installer default npm registry

zjffdu
In reply to this post by zjffdu
Github user 1ambda commented on the issue:

    https://github.com/apache/zeppelin/pull/2349
 
    @necosta Thanks for quick updating!
   
    I have one more question.  **Is there a proxy option for npm packages? (not npm itself)**
    Since if we an enable helium package, it will download specified (required) npm packages.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] zeppelin issue #2349: [ZEPPELIN-2214] Set npm installer default npm registry

zjffdu
In reply to this post by zjffdu
Github user necosta commented on the issue:

    https://github.com/apache/zeppelin/pull/2349
 
    @1ambda , thanks for the quick feedback :)
    This ticket has related ticket ZEPPELIN-2152 which has a recent comment related to your question:
    https://github.com/NohSeho/zeppelin/commit/032e120d06436e8b6c6b3b881efd20ba2c828fb2
    Not sure what is the status of it (if under review or not proposed as a pull request yet) but I think we can separate issues.
    This is, this ticket to configure the call to a repository manager, ticket ZEPPELIN-2152 to handle proxy configuration. What do you think?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] zeppelin issue #2349: [ZEPPELIN-2214] Set npm installer default npm registry

zjffdu
In reply to this post by zjffdu
Github user 1ambda commented on the issue:

    https://github.com/apache/zeppelin/pull/2349
 
    Yes, we can split. Then,
   
    - Let's make CI green
    - I left one comment.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] zeppelin issue #2349: [ZEPPELIN-2214] Set npm installer default npm registry

zjffdu
In reply to this post by zjffdu
Github user necosta commented on the issue:

    https://github.com/apache/zeppelin/pull/2349
 
    @1ambda, latest updates:
     - Renamed "registry" to "installer.url" as suggested in your comment
     - Fixed CI
     - "ZEPPELIN-2214 is about setting proxy for Helium. But Actually, this PR it not related with proxy itself. Just for configurable URLs." - True in a way... Should I create a new ticket? Or should we update ZEPPELIN-2214 description? What do you suggest? Thanks.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] zeppelin pull request #2349: [ZEPPELIN-2214] Set npm installer default npm r...

zjffdu
In reply to this post by zjffdu
Github user necosta closed the pull request at:

    https://github.com/apache/zeppelin/pull/2349


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] zeppelin pull request #2349: [ZEPPELIN-2214] Set npm installer default npm r...

zjffdu
In reply to this post by zjffdu
GitHub user necosta reopened a pull request:

    https://github.com/apache/zeppelin/pull/2349

    [ZEPPELIN-2214] Set npm installer default npm registry

    ### What is this PR for?
    Complements ZEPPELIN-2278, this is a fix for "connection timeout" bug when trying to run "npm install" behind a proxy. Instead of connecting to the official nodejs site from behind a proxy, we should use the already soft-coded env variable ZEPPELIN_HELIUM_NPM_REGISTRY repository.
   
    ### What type of PR is it?
    Bug Fix
   
    ### ToDos
    * [x] - Soft-code npm folder name as well (currently hardcoded "npm") - To be agreed on...
   
    ### What is the Jira issue?
    * [ZEPPELIN-2214 - Bug on connecting to the official nodejs site from behind a proxy]
   
    ### How should this be tested?
    Enable any Helium plugin from behind a proxy
   
    ### Questions:
    * Does the licenses files need update? No
    * Is there breaking changes for older versions? No
    * Does this needs documentation? No


You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/nokia/zeppelin zeppelin2214

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/zeppelin/pull/2349.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #2349
   
----
commit 9fbf9638a0f4647cf3642220c9a5ad908ac8dfe3
Author: Nelson Costa <[hidden email]>
Date:   2017-05-19T09:40:29Z

    [ZEPPELIN-2214] Set npm installer default npm registry

commit 877dd9d1c31bb2544109146f4a69045b3ba6e0bb
Author: Nelson Costa <[hidden email]>
Date:   2017-05-19T09:56:12Z

    [ZEPPELIN-2214] Fixed broken CI

commit c6fc912aeffcb4b917e17a8dbec2b59a5eb7ed33
Author: Nelson Costa <[hidden email]>
Date:   2017-05-22T07:33:13Z

    [ZEPPELIN-2214] Renamed variables

commit f27ce8a80551a33027ad78474b3981d84d0f7aa9
Author: Nelson Costa <[hidden email]>
Date:   2017-05-22T08:02:43Z

    [ZEPPELIN-2214] Fixed broken CI

commit 5010a8c32c7ad03bc13bb9b515dbd94b0cf13250
Author: Nelson Costa <[hidden email]>
Date:   2017-05-23T06:57:53Z

    [ZEPPELIN-2214] Renamed yarn to yarnpkg

----


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] zeppelin issue #2349: [ZEPPELIN-2214] Set npm installer default npm registry

zjffdu
In reply to this post by zjffdu
Github user 1ambda commented on the issue:

    https://github.com/apache/zeppelin/pull/2349
 
    Personally, I hope we have a proxy option instead of configurations for those URLs.
    That's because, it's hard to configure every URLs for users. (not scalable solution) There might be more URLs except for helium related thins.  (e.g maven dep loader, `%spark.dep`, ...)
   
    If we have proxy support in Zeppelin, we just need to set that configuration only.
   
    > Does it make sense to use the well known environment variables http_proxy and no_proxy? They are usually configured correctly already on all the machines that require proxy settings. If there is a use case to have different proxy settings for the node installer and the npm installer (although I don't have one personally right now), we can still provide separate configuration properties. But it would certainly come in handy to use http_proxy for all of them if it is set and not overwritten.



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] zeppelin issue #2349: [ZEPPELIN-2214] Set npm installer default npm registry

zjffdu
In reply to this post by zjffdu
Github user necosta commented on the issue:

    https://github.com/apache/zeppelin/pull/2349
 
    Thanks @1ambda,
    Allow me to disagree :) In my opinion should be: "I hope we have a proxy option as well as configurations for those URLs"
    The advantage of having configurable URLs is that it allows us to setup a repository manager (like Nexus or JFrog Artifactory) for Zeppelin. Not having one, has disadvantages like no mirroring (leading to latency in download) and inability to have it running in a private cloud (the main reason I started working on this ticket). From what I have seen, the other URLs (maven dep) are handled already.
    I just created pull request https://github.com/apache/zeppelin/pull/2363 attached against ZEPPELIN-2152. Just looking at the unit tests it is now successfully using proxy env variables.
    So, to conclude, I would kindly request review on both pull requests. Thanks.
    P.S. If you still disagree, should we get a 3rd opinion? I think it would good. Thanks.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] zeppelin issue #2349: [ZEPPELIN-2214] Set npm installer default npm registry

zjffdu
In reply to this post by zjffdu
Github user 1ambda commented on the issue:

    https://github.com/apache/zeppelin/pull/2349
 
    If we have both, then it's better.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] zeppelin pull request #2349: [ZEPPELIN-2214] Set npm installer default npm r...

zjffdu
In reply to this post by zjffdu
GitHub user necosta reopened a pull request:

    https://github.com/apache/zeppelin/pull/2349

    [ZEPPELIN-2214] Set npm installer default npm registry

    ### What is this PR for?
    Complements ZEPPELIN-2278, this is a fix for "connection timeout" bug when trying to run "npm install" behind a proxy. Instead of connecting to the official nodejs site from behind a proxy, we should use the already soft-coded env variable ZEPPELIN_HELIUM_NPM_REGISTRY repository.
   
    ### What type of PR is it?
    Bug Fix
   
    ### ToDos
    * [x] - Soft-code npm folder name as well (currently hardcoded "npm") - To be agreed on...
   
    ### What is the Jira issue?
    * [ZEPPELIN-2214 - Bug on connecting to the official nodejs site from behind a proxy]
   
    ### How should this be tested?
    Enable any Helium plugin from behind a proxy
   
    ### Questions:
    * Does the licenses files need update? No
    * Is there breaking changes for older versions? No
    * Does this needs documentation? No


You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/nokia/zeppelin zeppelin2214

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/zeppelin/pull/2349.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #2349
   
----
commit 9fbf9638a0f4647cf3642220c9a5ad908ac8dfe3
Author: Nelson Costa <[hidden email]>
Date:   2017-05-19T09:40:29Z

    [ZEPPELIN-2214] Set npm installer default npm registry

commit 877dd9d1c31bb2544109146f4a69045b3ba6e0bb
Author: Nelson Costa <[hidden email]>
Date:   2017-05-19T09:56:12Z

    [ZEPPELIN-2214] Fixed broken CI

commit c6fc912aeffcb4b917e17a8dbec2b59a5eb7ed33
Author: Nelson Costa <[hidden email]>
Date:   2017-05-22T07:33:13Z

    [ZEPPELIN-2214] Renamed variables

commit f27ce8a80551a33027ad78474b3981d84d0f7aa9
Author: Nelson Costa <[hidden email]>
Date:   2017-05-22T08:02:43Z

    [ZEPPELIN-2214] Fixed broken CI

commit 5010a8c32c7ad03bc13bb9b515dbd94b0cf13250
Author: Nelson Costa <[hidden email]>
Date:   2017-05-23T06:57:53Z

    [ZEPPELIN-2214] Renamed yarn to yarnpkg

commit 8448a93ae2d36af0f3907e22c1bfc65cdd8a9664
Author: Nelson Costa <[hidden email]>
Date:   2017-05-24T07:07:14Z

    [ZEPPELIN-2214] Renamed npm registry to npm installer url

----


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] zeppelin pull request #2349: [ZEPPELIN-2214] Set npm installer default npm r...

zjffdu
In reply to this post by zjffdu
Github user necosta closed the pull request at:

    https://github.com/apache/zeppelin/pull/2349


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] zeppelin issue #2349: [ZEPPELIN-2214] Set npm installer default npm registry

zjffdu
In reply to this post by zjffdu
Github user necosta commented on the issue:

    https://github.com/apache/zeppelin/pull/2349
 
    @1ambda ,
    Renamed "zeppelin.helium.npm.registry" to "zeppelin.helium.npm.installer.url"
    I understand there is no need to update file "upgrade.md" as the change in conf value is inside the same version (0.8)
    Ready for review.  Thanks


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] zeppelin issue #2349: [ZEPPELIN-2214] Set npm installer default npm registry

zjffdu
In reply to this post by zjffdu
Github user necosta commented on the issue:

    https://github.com/apache/zeppelin/pull/2349
 
    Hi @1ambda and @Leemoonsoo ,
    Can you have another look at this one?
    After rebasing latest master (now contains [ZEPPELIN-2152](https://issues.apache.org/jira/browse/ZEPPELIN-2152)) and squashing my commits, this is the version I would like to get approved.
    There is a lot of info. in the above comments. Any question, let me know and thank you.
    By the way, I linked [ZEPPELIN-2227](https://issues.apache.org/jira/browse/ZEPPELIN-2227) as duplicate to this one ([ZEPPELIN-2214](https://issues.apache.org/jira/browse/ZEPPELIN-2214)).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] zeppelin issue #2349: [ZEPPELIN-2214] Set npm installer default npm registry

zjffdu
In reply to this post by zjffdu
Github user necosta commented on the issue:

    https://github.com/apache/zeppelin/pull/2349
 
    ping


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] zeppelin issue #2349: [ZEPPELIN-2214] Set npm installer default npm registry

zjffdu
In reply to this post by zjffdu
Github user Leemoonsoo commented on the issue:

    https://github.com/apache/zeppelin/pull/2349
 
    Tested and LGTM. Thanks @necosta for the improvement!
    Merge to master if no further comment.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
12