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

Refactor Dockerfile docs #3309

Merged
merged 1 commit into from Mar 25, 2022
Merged

Refactor Dockerfile docs #3309

merged 1 commit into from Mar 25, 2022

Conversation

Kevan-Y
Copy link
Contributor

@Kevan-Y Kevan-Y commented Mar 24, 2022 •
edited

Issue This PR Addresses

Fixes #1668

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

  • Add Docker Healthcheck
  • Remove development part

Steps to test the PR

  • cd src/docs
  • docker build -t docs:test .
  • docker run --rm -p 80:80 docs:test
  • Go to localhost

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)

@Kevan-Y Kevan-Y self-assigned this Mar 24, 2022
@vercel
Copy link

vercel bot commented Mar 24, 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/7SKEUjgDYGRMAvY9dSF1QxoS7H2R
✅ Preview: Failed

@gitpod-io
Copy link

gitpod-io bot commented Mar 24, 2022

@Kevan-Y Kevan-Y mentioned this pull request Mar 24, 2022
Closed
@Kevan-Y Kevan-Y added this to the 2.9 Release milestone Mar 24, 2022
tcvan0707
tcvan0707 reviewed Mar 25, 2022
View reviewed changes
Copy link
Contributor

@tcvan0707 tcvan0707 left a comment

Choose a reason for hiding this comment

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

I just pulled your branch and tested locally, it was the same as how we normally run the docs pnpm start. Is that how we expect this PR to run?

src/docs/Dockerfile

# Add Healthcheck
HEALTHCHECK --interval=30s --timeout=30s --start-period=5s --retries=3 \
CMD curl --fail localhost:80 || exit 1
Copy link
Contributor

Choose a reason for hiding this comment

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

One thing I want to clarify. At first, I didn't know which localhost should I go to (based on what you have in your Steps to test the PR), then I tried to specify localhost:80 and it was successful. Is localhost:80 same as localhost (when we type in the URL)?

Copy link
Contributor

Choose a reason for hiding this comment

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

localhost and localhost:80 are the same, correct.

@Kevan-Y
Copy link
Contributor Author

Kevan-Y commented Mar 25, 2022 •
edited

I just pulled your branch and tested locally, it was the same as how we normally run the docs pnpm start. Is that how we expect this PR to run?

No, pnpm start in docs will run the development side of docusaurus,
Here we generate the static site, then we put it under nginx to server our static content

To run this PR, you can see the step put in the description of the PR,

I'm putting it here in case you missed it.

Steps to test the PR

  1. cd src/docs
  2. docker build -t docs:test .
  3. docker run --rm -p 80:80 docs:test
  4. Go to localhost

menghif
menghif approved these changes Mar 25, 2022
View reviewed changes
Copy link
Contributor

@menghif menghif left a comment

Choose a reason for hiding this comment

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

It works

@Kevan-Y Kevan-Y merged commit c7e295d into Seneca-CDOT:master Mar 25, 2022
@cindyledev
Copy link
Contributor

Interesting to see this change only triggers prettier and eslint ci. We want to know if the Dockerfile change builds correctly.

I think we need to update the workflow so telescope/src/docs/docs (or src/web/docusaurus/docs after we move the docs directory into src/web) triggers prettier and eslint ci.

@humphd
Copy link
Contributor

humphd commented Mar 25, 2022

Agree, https://github.com/Seneca-CDOT/telescope/blob/master/.github/workflows/continuous-integration.yml#L8 is wrong now. @cindyledev could you file an issue?

@cindyledev
Copy link
Contributor

Agree, https://github.com/Seneca-CDOT/telescope/blob/master/.github/workflows/continuous-integration.yml#L8 is wrong now. @cindyledev could you file an issue?

No problem. I'll file it now

@cindyledev cindyledev mentioned this pull request Mar 25, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@humphd humphd humphd left review comments

@menghif menghif menghif approved these changes

@tcvan0707 tcvan0707 tcvan0707 approved these changes

@cindyledev cindyledev Awaiting requested review from cindyledev

@joelazwar joelazwar Awaiting requested review from joelazwar

@JerryHue JerryHue Awaiting requested review from JerryHue

@TDDR TDDR Awaiting requested review from TDDR

@aserputov aserputov Awaiting requested review from aserputov

Assignees

@Kevan-Y Kevan-Y

Labels
area: docker
Projects
None yet
Milestone
2.9 Release
Development

Successfully merging this pull request may close these issues.

Apply Dockerfile best practices from Snyk recommendations
6 participants
@Kevan-Y @cindyledev @humphd @menghif @tcvan0707 @AmasiaNalbandian