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

Split Dockerfile for production #4292

Closed
wants to merge 6 commits into from
Closed

Split Dockerfile for production #4292

wants to merge 6 commits into from

Conversation

koooge
Copy link
Contributor

@koooge koooge commented Oct 26, 2019

What type of PR is this? (check all applicable)

  • Refactor

Description

Hi there.
This PR splits Dockerfile for production(Dockerfile) from development(Dockerfile.dev) to reduce dev dependencies(requirements_dev) influence and size.

Related Tickets & Documents

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

@koooge koooge changed the title Split Dockerfile for production wip: Split Dockerfile for production Oct 26, 2019
@koooge koooge changed the title wip: Split Dockerfile for production Split Dockerfile for production Oct 26, 2019
Copy link
Member

@arikfr arikfr left a comment

Choose a reason for hiding this comment

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

Thanks. I was thinking of creating a separate file for development for quite some time now.

See comments.

requirements.txt Outdated Show resolved Hide resolved
Dockerfile.dev Outdated
Comment on lines 1 to 10
FROM node:10 as frontend-builder

WORKDIR /frontend
COPY package.json package-lock.json /frontend/
RUN npm install

COPY client /frontend/client
COPY webpack.config.js /frontend/
RUN npm run build

Copy link
Member

Choose a reason for hiding this comment

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

Now that we have a dedicated Docker file for development we no longer need this here, as we build frontend assets on the host.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By deleted this build step, then the some tests which depend on client/dist/ became to fail. So I changed index handler to return 200 with error.html in that TemplateNotFound case 6ab16ec

.dockerignore Outdated Show resolved Hide resolved
.circleci/config.yml Outdated Show resolved Hide resolved
Dockerfile.dev Show resolved Hide resolved
requirements.txt Outdated Show resolved Hide resolved
redash/handlers/static.py Outdated Show resolved Hide resolved
build: ../
build:
context: ../
dockerfile: Dockerfile.dev
Copy link
Member

Choose a reason for hiding this comment

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

In CircleCI we don't build assets outside of the Docker container, so I wonder how the the tests are passing? Is it because of the exception handling in static.py?

Copy link
Contributor Author

@koooge koooge Nov 14, 2019

Choose a reason for hiding this comment

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

Right. Some test cases of backend depend on the build assets.

message = "Missing template file ({}). Did you build the frontend assets with npm?".format(e.name)
else:
message = "Error Rendering Page."
response = render_template("error.html", error_message=message)
Copy link
Member

Choose a reason for hiding this comment

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

I realized that this needs to be a response with status code 500.

Copy link
Contributor Author

@koooge koooge Nov 14, 2019

Choose a reason for hiding this comment

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

Yes, actually it should return 404 or 500. But it returns 200 due to backend test 🤔
#4292 (comment)

sudo apt install -y nodejs
npm install
npm run build
docker cp ./client/dist ${COMPOSE_PROJECT_NAME}_redash_1:/app/client/
Copy link
Contributor Author

@koooge koooge Nov 15, 2019

Choose a reason for hiding this comment

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

Build Frontend For Backend Test! 😢

koooge added 6 commits April 28, 2020 21:18
Signed-off-by: koooge <koooooge@gmail.com>
Signed-off-by: koooge <koooooge@gmail.com>
Signed-off-by: koooge <koooooge@gmail.com>
Signed-off-by: koooge <koooooge@gmail.com>
Signed-off-by: koooge <koooooge@gmail.com>
Signed-off-by: koooge <koooooge@gmail.com>
@koooge
Copy link
Contributor Author

koooge commented May 12, 2020

Superseded by #4879

@koooge koooge closed this May 12, 2020
@koooge koooge deleted the test/split_container branch May 12, 2020 15: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
Development

Successfully merging this pull request may close these issues.

3 participants