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

[docker] fix, Dockerfile for frontend builds #9114

Merged
merged 8 commits into from
Feb 12, 2020

Conversation

suddjian
Copy link
Member

@suddjian suddjian commented Feb 10, 2020

CATEGORY

Choose one

  • Bug Fix
  • Enhancement (new features, refinement)
  • Refactor
  • Add tests
  • Build / Development Environment
  • Documentation

SUMMARY

My previous PR introduced a bug to the Dockerfile. This fixes it by copying build frontend files to/from the right places.

TEST PLAN

I've verified this by running docker-compose up --build, but again would appreciate verification from someone better versed in docker-ology

ADDITIONAL INFORMATION

REVIEWERS

@mistercrunch @nytai

@codecov-io
Copy link

codecov-io commented Feb 10, 2020

Codecov Report

Merging #9114 into master will decrease coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #9114      +/-   ##
=========================================
- Coverage    59.1%   59.1%   -0.01%     
=========================================
  Files         372     372              
  Lines       11920   11922       +2     
  Branches     2917    2919       +2     
=========================================
+ Hits         7045    7046       +1     
- Misses       4693    4694       +1     
  Partials      182     182
Impacted Files Coverage Δ
superset-frontend/src/chart/chartAction.js 43.33% <0%> (+0.09%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2913063...9bc622a. Read the comment docs.

Copy link
Member

@nytai nytai left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Member

@nytai nytai left a comment

Choose a reason for hiding this comment

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

actually getting:
ERROR: Service 'superset' failed to build: COPY failed: stat /var/lib/docker/overlay2/da84b52e48a6b3377e530754d4fd271917761b201d64bc3a77d4e8574971ba62/merged/app/superset-frontend/version_info.json: no such file or directory when running locally

@nytai
Copy link
Member

nytai commented Feb 11, 2020

Looks like version_info.json is produced by running setup.py. I think it's only meant to end up in the actual pip tarball package under superset/static/assets/version_info.json. Looking at setup.py I'm not sure the paths are correct for generating version_info.json.

@suddjian
Copy link
Member Author

I think you're right, thanks for catching that @nytai

@suddjian
Copy link
Member Author

suddjian commented Feb 11, 2020

Whew. Had some trials and tribulations as you can see from the commit log but I dare say this is ready to go. Approvals/criticisms welcome.

One change I was forced to make was moving version_info.json from /static/assets to /static. Issues arose around it being built by python code while webpack owns the contents of /assets. version_info isn't really an asset anyway, so I would argue that /static is a more correct location for it in this directory structure.

@@ -26,6 +26,7 @@ x-superset-volumes: &superset-volumes
- ./docker/docker-init.sh:/app/docker-init.sh
- ./docker/pythonpath_dev:/app/pythonpath
- ./superset:/app/superset
- ./superset-frontend:/app/superset-frontend
Copy link
Member

Choose a reason for hiding this comment

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

👍

Dockerfile Outdated Show resolved Hide resolved
@nytai
Copy link
Member

nytai commented Feb 12, 2020

should also fix #9108, #9116

Copy link
Member

@dpgaspar dpgaspar left a comment

Choose a reason for hiding this comment

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

Looks good, needs a possible change

Dockerfile Show resolved Hide resolved
@dpgaspar dpgaspar changed the title fix Dockerfile for frontend builds [docker] fix, Dockerfile for frontend builds Feb 12, 2020
Copy link
Member

@dpgaspar dpgaspar left a comment

Choose a reason for hiding this comment

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

Gave it a quick run and looks good

@dpgaspar dpgaspar merged commit 966d321 into apache:master Feb 12, 2020
@Taki-Eddine Taki-Eddine mentioned this pull request Feb 12, 2020
@suddjian suddjian deleted the frontend-docker-fix branch February 13, 2020 05:42
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.36.0 labels Feb 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/XS 🚢 0.36.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Superset-node docker compose fails to create container
5 participants