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

Update setup.py script to fix installation issue #85

Merged
merged 1 commit into from
Apr 18, 2019

Conversation

feng-tao
Copy link
Member

@feng-tao feng-tao commented Apr 17, 2019

I encountered the same issue mentioned by @verdan in #82 with a fresh checkout of the repo. The issue seems to be related to using npm install --only==prod.

I don't have much context as @danwom and @ttannis on this flag. My understanding is to save the build size to reduce page load time? But I don't think page load time is really an issue with the tool.

Per heartsucker/node-deb#15 , it will skip installing those devDependencies listed in package.json.

By removing the --only==prod, the python setup.py install command works at my local setup.

@feng-tao
Copy link
Member Author

cc @verdan @danwom @ttannis

@verdan
Copy link
Member

verdan commented Apr 17, 2019

This looks good to me, as with the --only==prod you don't install linting packages, which are required for the build command.
Is it possible to release on PyPi once we fix this issue, as we need the pip package to deploy amundsen within our team.

@feng-tao
Copy link
Member Author

@verdan will do once I get a +1. Will ping you once I do a push to pypi.

@feng-tao
Copy link
Member Author

cc @jinhyukchang

@ttannis
Copy link
Contributor

ttannis commented Apr 17, 2019

Some initial context: is_npm_installed() and build_js() methods were a part of some experimental code when I was initially investigating how to manage the fact that we needed to build assets with npm commands yet generate a working python package. Upfront:

  1. These methods should have been deleted
  2. I saw some comment in a previous PR about wanting the built assets in the dist to be included in the python package? If so, the intent here is that users are still supposed to be running npm install and npm run build in their own local environments to generate the application source.

More context on what the goal is would be helpful in order to know how to move forward.

@feng-tao
Copy link
Member Author

feng-tao commented Apr 17, 2019 via email

@verdan
Copy link
Member

verdan commented Apr 17, 2019

@ttannis, the use case is to run the amundsen project using pip without cloning and building the assets manually. I believe compiled assets should be a part of each pip release.
I've started a thread on slack channel to discuss how people would deploy amundsen in production, and explained a bit of my structure.

https://amundsenworkspace.slack.com/archives/CGFBVT23V/p1555522967029000

@bolkedebruin
Copy link
Member

It is probably smart to keep an eye on the Licenses of the dependencies you are pulling in (npm and pip) and making part of the package. The pro of building locally is that you are not redistributing which could complicate things for example in case of GPLed libs.

@jinhyukchang jinhyukchang merged commit 8256454 into master Apr 18, 2019
@jinhyukchang
Copy link
Contributor

@verdan v.1.0.2 has been released.
https://pypi.org/project/amundsen-frontend/

@verdan
Copy link
Member

verdan commented Apr 18, 2019

thanks @jinhyukchang
I confirm that this is working just fine now.

@danwom danwom deleted the tfeng_update_setup branch April 23, 2019 22:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants