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

Draft: use multi-staged build for clean frontend image #348

Closed
wants to merge 1 commit into from

Conversation

PascalinDe
Copy link
Contributor

closes #342

@PascalinDe PascalinDe requested a review from a team as a code owner September 8, 2023 12:52
@PascalinDe PascalinDe changed the title fix: use multi-staged build for clean frontend image Draft: use multi-staged build for clean frontend image Sep 8, 2023
Copy link
Member

@ineiti ineiti left a comment

Choose a reason for hiding this comment

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

Did you try that? I'm surprised that npm start works without re-compiling everything.

Comment on lines 10 to 14
FROM node:20-bookworm as app

COPY --from=build /web/frontend/ .

ENTRYPOINT ["npm", "start"]
Copy link
Member

Choose a reason for hiding this comment

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

Does that work? You don't need nodejs anymore, as the npm run build will create just a set of static html/js/css files. So you can do:

https://github.com/c4dt/service-stainless/blob/main/webapp/Dockerfile#L11

Or

c4dt/service-stainless@bfeac27#diff-06839e9bf05123a2ab7c9a5d721be661be9d231a6337bc7b8b0b6d8d8bf53d44R11

Or any other lightweight web server image.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was testing it and it initially seemed fine, but now it's not working, so I put it back into Draft

before that it sort of complained that there was no package.json file as npm ci does not seem to generate one, so I copied everything over to test that

thanks for the comments, I think the error is in there somewhere (I was looking at the Dockerfiles just now, I think https://github.com/c4dt/service-spindle/blob/main/Dockerfile is probably the closest)

Copy link
Member

Choose a reason for hiding this comment

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

I was looking at the Dockerfiles

The spindle Dockerfile uses the node image, which is not necessary, as you should only have static files to serve for the frontend. So either a python or a webserver for static content is enough. Here is another, even more minimal webserver:

https://lipanski.com/posts/smallest-docker-image-static-website

@PascalinDe
Copy link
Contributor Author

the frontend UI seems to be working with this version now, but I'm having trouble receiving data from DELA now

@PascalinDe
Copy link
Contributor Author

OK, working now

@PascalinDe PascalinDe changed the title Draft: use multi-staged build for clean frontend image refactor: use multi-staged build for clean frontend image Sep 8, 2023
WORKDIR /web/frontend
COPY --from=build /web/frontend/build/ .

ENTRYPOINT ["python3", "-m", "http.server", "3000", "--directory", "/web/frontend/", "--bind", "127.0.0.1"]
Copy link
Contributor

Choose a reason for hiding this comment

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

I see you removed npm start in favor of building the static site. There's one caveat to that, which is that during development, you will need to rebuild every time you do a change.

With no npm start, you can get hot reloading, or at least you would need to do docker compose restart container
With this design, you would need docker .... build first then docker .... up second.

but... maybe, if we change the entrypoint to a CMD, we can override it in compose file to a npm run

Copy link
Contributor Author

Choose a reason for hiding this comment

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

please see @ineiti 's comments, now that we want to actually deploy it I agree that it makes sense to have a clean Docker image

personally, I have a separate Dockerfile for developing that kept the npm start for the reasons you mentioned

Copy link
Member

Choose a reason for hiding this comment

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

You can also add a Dockerfile.dev with the npm start in it.

For the npm start, do you use the -v to link your local directory?

Comment on lines +10 to +15
FROM node:20-bookworm as app

WORKDIR /web/frontend
COPY --from=build /web/frontend/build/ .

ENTRYPOINT ["python3", "-m", "http.server", "3000", "--directory", "/web/frontend/", "--bind", "127.0.0.1"]
Copy link
Contributor

Choose a reason for hiding this comment

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

why use python (and the node image) instead of a webserver, since that's the only thing we need? 🤔

@pierluca
Copy link
Contributor

PS: there are a couple of Sonar code smells that are trivial to address (s/ as / AS /g)

@sonarcloud
Copy link

sonarcloud bot commented Sep 25, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@PascalinDe PascalinDe changed the title refactor: use multi-staged build for clean frontend image Draft: use multi-staged build for clean frontend image Sep 25, 2023
@PascalinDe PascalinDe marked this pull request as draft September 25, 2023 14:26
@PascalinDe
Copy link
Contributor Author

the frontend server acts as a reverse proxy, so there needs to be a server running here as well - we'll shelve this PR for now

@PascalinDe PascalinDe closed this Nov 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: To do
Development

Successfully merging this pull request may close these issues.

Docker-image creation
4 participants