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

Jump to bottom

Fix #2993: Share single nginx container for all static sites #3237

Closed
wants to merge 4 commits into from
Closed

Fix #2993: Share single nginx container for all static sites #3237

wants to merge 4 commits into from

Conversation

menghif
Copy link
Contributor

@menghif menghif commented Mar 17, 2022

Issue This PR Addresses

Fixes #2993

Type of Change

  • Bugfix: Change which fixes an issue
  • New Feature: Change which adds functionality
  • Documentation Update: Change which improves documentation
  • UI: Change which improves UI

Description

Draft PR to share nginx Docker container for all static sites (web app, docusaurus, test-content).

As per issue (and @cindyledev's comment) I have restructured the projects like this:

src/web/Dockerfile
src/web/app
src/web/docusaurus
src/web/test-content

Steps to test the PR

  • pnpm i
  • pnpm services:start
  • go to localhost:8000 for telescope app
  • go to localhost:4631 for docusaurus app

Checklist

  • Quality: This PR builds and passes our npm test and works locally
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not (if applicable)
  • Documentation: This PR includes updated/added documentation to user exposed functionality or configuration variables are added/changed or an explanation of why it does not(if applicable)

@gitpod-io
Copy link

gitpod-io bot commented Mar 17, 2022

@vercel
Copy link

vercel bot commented Mar 17, 2022 •
edited

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/humphd/telescope/Gk8iBThkxYpWsSP5qgxVPN6usQ6o
✅ Preview: https://telescope-git-fork-menghif-issue-2993-humphd.vercel.app

@menghif menghif self-assigned this Mar 17, 2022
Kevan-Y
Kevan-Y requested changes Mar 18, 2022
View reviewed changes
src/web/Dockerfile
Comment on lines 21 to 22
RUN apk add --no-cache tini util-linux
ENTRYPOINT [ "/sbin/tini", "--"]
Copy link
Contributor

Choose a reason for hiding this comment

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

tini probably not needed anymore since we can use init: true in docker-compose

config/nginx.conf.development.template
Comment on lines +183 to +187
server {
listen 4631 default_server;
server_name localhost;
root /usr/share/nginx/html;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

What would be the route in production? we don't want to expose 4631 in production

@JerryHue JerryHue self-requested a review March 22, 2022 12:41
@JerryHue JerryHue closed this Mar 22, 2022
@menghif
Copy link
Contributor Author

menghif commented Mar 22, 2022

Closed in favour of breaking this into 2 PRs:

  • First to restructure the repo to include all static sites into src/web
  • Second to add a new Dockerfile for the Ngix shared config

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Kevan-Y Kevan-Y Kevan-Y requested changes

@AmasiaNalbandian AmasiaNalbandian Awaiting requested review from AmasiaNalbandian

@JerryHue JerryHue Awaiting requested review from JerryHue

Assignees

@menghif menghif

Labels
area: docker
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Share single nginx container for all static sites
3 participants
@menghif @Kevan-Y @JerryHue