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

Fixes #2792 Set up Docusaurus as a Microservice #2794

Closed
wants to merge 9 commits into from

Conversation

cindyorangis
Copy link
Contributor

@cindyorangis cindyorangis commented Feb 2, 2022

Issue This PR Addresses

Fixes #2792
Fixes #2746
Fixes #290

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

This sets up Docusaurus in a Docker container that runs locally on port 4631.

So far, I only have the Docusaurus website running in a Docker container. To test the container:

  1. Add my repository as a remote: git remote add cindyledev https://github.com/cindyledev/telescope.git
  2. Fetch my branches: git fetch cindyledev
  3. Checkout my branch: git checkout docusaurus
  4. Change directory to /src/docs: cd src/docs
  5. Build the Docker image: docker build --target development -t docs:dev .
  6. Run the Docker container: docker run -p 4631:4631 docs:dev
  7. Go to localhost:4631 to see the Docusaurus website.

TODO
Nginx stuff


How to test this PR

  1. Add my repository as a remote: git remote add cindyledev https://github.com/cindyledev/telescope.git
  2. Fetch my branches: git fetch cindyledev
  3. Checkout my branch: git checkout docusaurus
  4. Install dependencies: pnpm install
  5. Start the Docker containers: pnpm services:start

Go to localhost:4631 to see the Docusaurus website.

Steps to test the PR

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 Feb 2, 2022

Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

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

These eslint errors will require you to add a new section to our .eslintrc.js file in the root under the overrides section that defines what to do for src/api/docs/**/*.js. Use the src/web or src/api/status as an example.

Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

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

Let's also change the routing to /docs, so you'd go to telescope.cdot.systems/docs to see all this.

This is exciting to see happening.

.eslintrc.js Outdated Show resolved Hide resolved
@aserputov
Copy link
Contributor

@cindyledev can I help you somehow ?

@manekenpix
Copy link
Member

Should we add src/docs to pnpm-workspaces.yaml?

@manekenpix
Copy link
Member

manekenpix commented Feb 3, 2022

Accessing it via traefik shows 404. With a bit of tweaking, I managed to access it using localhost/docs (behind traefik), but all I could get was
image

Changing baseUrl breaks the build. Since this is not an API, would it make sense to run it just behind nginx and not behind nginx and traefik? I think nginx could just serve the docs as static content. as we do with our front-end (and we wouldn't have to use docusaurus's server).

config/env.production Outdated Show resolved Hide resolved
config/env.staging Outdated Show resolved Hide resolved
- DOCS_URL
depends_on:
- traefik
labels:
Copy link
Contributor

Choose a reason for hiding this comment

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

If we do this with nginx, we can drop all this, and have docs depend only on nginx.

src/docs/README.md Outdated Show resolved Hide resolved
Comment on lines 38 to 40
FROM nginx:stable as deploy
# https://github.com/krallin/tini
RUN apk --no-cache add tini
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm having problems building the docker container through pnpm run services:start.

I am not very familiar with docker, but I think these are the problematic parts. The deploy image we are using uses debian instead of alpine, which makes the line RUN apk ... to fail: apk does not exist. I tried to change it to apt-get install tini, but the package cannot be found (despite the tini GitHub repo saying otherwise...).

Maybe we could use an alpine image when building everything, and then place the nginx image right before the COPYing step, just like we do in src/web/Dockerfile?

Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need tini at all, we can get rid of this completely, since we aren't actually running a process. tini is for running the node process in a container. In this case, we build the site with node, then copy the results into the nginx container's filesystem where it expects to find it.

We could change nginx:stable to nginx:stable-alpine though, to save some size.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the reviews. I'm not a Docker pro so I copied and pasted some of @humphd work from #2808 . Also removed tini and changed nginx:stable to nginx:stable-alpine

@Kevan-Y
Copy link
Contributor

Kevan-Y commented Feb 4, 2022

Wasn't able to run got this issue,
image

I asked for some help @humphd answered with

COPY --from=deploy /home/node/app/build /var/www/data
that is trying to copy something from the deploy stage into the deploy stage
get rid of --from=deploy
get rid of RUN apk --no-cache add tini too (edited) 
this needs to be reworked

But wasn't able to run it probably need a bit of rework

Did Dave review this in the Docker/Nginx meeting on Feb 4th? I'm so sad I missed it and I'm even more sad that no one recorded it! :(

I copied and pasted some stuff from our /src/web/Dockerfile hence the
COPY --from=deploy /home/node/app/build /var/www/data cough cough. You think I'd know what I'm copying but not all the time... I updated it to

# Copy build output to /var/www/data to be served
COPY /home/node/app/build /var/www/data

@cindyorangis
Copy link
Contributor Author

Steps to test the Docusarus website Docker container

  1. Add my repository as a remote: git remote add cindyledev https://github.com/cindyledev/telescope.git
  2. Fetch my branches: git fetch cindyledev
  3. Checkout my branch: git checkout docusaurus
  4. Change directory to /src/docs: cd src/docs
  5. Build the Docker image: docker build --target development -t docs:dev .
  6. Run the Docker container: docker run -p 4631:4631 docs:dev
  7. Go to localhost:4631 to see the Docusaurus website.

@cindyorangis
Copy link
Contributor Author

Wow apparently my PR is so bad that CI checks don't even wanna run tests for it =_=

@humphd
Copy link
Contributor

humphd commented Feb 6, 2022

@cindyledev this PR is pretty big, and I'm not 100% if you're ready for review? Let me know what help you need/want and when.

@JerryHue
Copy link
Contributor

JerryHue commented Feb 6, 2022

I wonder if we can break down the PR so that it is easier for reviewing?

In the issue #2792, I was expecting just Docusaurus to be installed in the project, and then later address the dockerization in a future issue/PR.

If that's not an option, then @cindyledev, would you accept if we make a PR on your branch? That way, we won't have to throw away the work you did.

@cindyorangis
Copy link
Contributor Author

I wonder if we can break down the PR so that it is easier for reviewing?

In the issue #2792, I was expecting just Docusaurus to be installed in the project, and then later address the dockerization in a future issue/PR.

If that's not an option, then @cindyledev, would you accept if we make a PR on your branch? That way, we won't have to throw away the work you did.

I will slim down this PR tomorrow because you're right, this PR is supposed to "Set up Docusaurus", I went too far out of scope which made it extremely difficult to review.

@cindyorangis
Copy link
Contributor Author

Thank you everyone for your feedback here. Closing this one in favour of #2853. Follow up issues will be filed later

@cindyorangis cindyorangis removed this from the 2.7 Release milestone Feb 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Set up Docusaurus Create the Docs Microservice Convert our GitHub docs to MDX in Next.js
6 participants