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

Simplifying docker contrib #5802

Closed
wants to merge 2 commits into from
Closed

Simplifying docker contrib #5802

wants to merge 2 commits into from

Conversation

kakoni
Copy link
Contributor

@kakoni kakoni commented Sep 4, 2018

  1. Removing docker-build.sh step and building superset container with docker-compose (build command)
  2. Removing exposed ports for psql and redis.
  3. Adding new SUPERSET_ENV development that also runs npm watcher
  4. Removing npm and superset runserver/worker from docker-init. Dockerfile already builds assets and starts superset.

@codecov-io
Copy link

codecov-io commented Sep 4, 2018

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5802      +/-   ##
==========================================
+ Coverage   63.82%   63.83%   +<.01%     
==========================================
  Files         364      364              
  Lines       23099    23100       +1     
  Branches     2587     2587              
==========================================
+ Hits        14744    14745       +1     
  Misses       8340     8340              
  Partials       15       15
Impacted Files Coverage Δ
superset/assets/src/explore/controls.jsx 46.26% <0%> (ø) ⬆️
superset/utils.py 89.16% <0%> (+0.01%) ⬆️

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 d43813f...2f67a04. Read the comment docs.

@ivansabik
Copy link

ivansabik commented Sep 4, 2018

Would it be worth adding some explicit instructions in the doc of how to run this locally using docker? Since it's not just docker-compose up. This is what works for me locally:

cp contrib/docker/{docker-build.sh,docker-compose.yml,docker-entrypoint.sh,docker-init.sh,Dockerfile} .
cp contrib/docker/superset_config.py superset/
bash -x docker-build.sh
docker-compose up -d
docker-compose exec superset bash
bash docker-init.sh

@kakoni
Copy link
Contributor Author

kakoni commented Sep 5, 2018

@ivansabik I thought that it already exists?
https://github.com/apache/incubator-superset/blob/master/docs/installation.rst#user-content-start-with-docker

Oh course if this PR gets merged that documentation has to changed a little.

@mistercrunch
Copy link
Member

@kakoni mind adjusting the docs as you mentionned as part of this PR?

@kakoni
Copy link
Contributor Author

kakoni commented Sep 24, 2018

@mistercrunch I thought I already did?;) 2f67a04

@victornoel
Copy link
Contributor

@kakoni this is potentially superseded by this PR: #5966.
Could you take a look and see if it does indeed? I am sorry I didn't see your PR before. Anyway mine goes farther but seems to tackle what you did.

@kakoni
Copy link
Contributor Author

kakoni commented Oct 1, 2018

@victornoel Hi! We seem to have same goals, so we should combine our work.
Anyway question; Build processes/docker images for develoment / production would look bit different (for instance running editable pip install, npm/webpack dev server running for assets). So perhaps, would it make sense to create two compose files(one for dev and one for prod like builds) ?

@victornoel
Copy link
Contributor

@kakoni from what I understood, nobody uses this image for production, most people are using https://hub.docker.com/r/amancevice/superset/, so for now, I wouldn't bother tackling this question.

In #5966, the objective is clearly to simplify the experience of the developer, both for the backend and the frontend.
Some things that differs in your PR:

  • your build the frontend in the entrypoint.sh. My take was to build it once in the image, and if the desire was to do development, the dev would run dev-server on its host, not in docker (that's not needed to do it in docker since running dev-server is happening all in memory). That being said, it means that if the frontend code change but the dev does not want to run dev-server, then he has to rebuild the image… that's maybe not the best, we could include building the front as part of starting the image.
  • yours build the image directly from docker-compose while mine lets the dev build it himself. I think you solution is more elegant.

WDYT? Would you agree that I incorporate those changes in my PR and that we continue with it? Since it covers more things too.

@brylie
Copy link
Contributor

brylie commented Oct 1, 2018

Also, is there an official docker image published on DockerHub? That would be helpful, so I can ask our DevOps architect how to set up a Superset instance in our AWS infrastructure.

@victornoel
Copy link
Contributor

@brylie you should open another issue for this, I don't think this is the best place to discuss it, @kakoni and I have no official position in the superset org :)
You can check out https://hub.docker.com/r/amancevice/superset/ also.

@kakoni kakoni changed the title Simpifying docker contrib Simplifying docker contrib Oct 1, 2018
@kakoni
Copy link
Contributor Author

kakoni commented Oct 1, 2018

@victornoel

Would you agree that I incorporate those changes in my PR and that we continue with it? Since it covers more things too.

Makes sense. Lets continue there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants