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

Added docker deployment #9263

Open
wants to merge 121 commits into
base: master
Choose a base branch
from
Open

Added docker deployment #9263

wants to merge 121 commits into from

Conversation

kurtisassad
Copy link
Contributor

@kurtisassad kurtisassad commented Sep 18, 2024

Link to Issue

Closes: #9085
Closes: #9158

Description of Changes

  • Refactors Dockerfiles to be compatible with the heroku docker deploys
  • Adds datadog to Dockerfile.web (corresponds to our app)
  • Adds deployment CD pipeline
  • Adds dockerfiles for snapshot listener + discordbot

Test Plan

  • Changed frack to use container deployments (step 1 of deployment plan)
  • Merged this branch into the frack branch which makes a deployment to commonwealth-frack
  • CAed around app
  • Checked that knock notifications work correctly

Deployment Plan

  1. When ready, first switch the heroku app from regular deploys to container deploys (heroku stack:set container -a commonwealthapp)
  2. Make a PR into production branch. After this is merged it will be deployed to heroku. This will be our new deployment process.

Other considerations

  • We need to do load testing. I needed to remove the get-max-old-space-size.sh because the docker image does not have information about the machine it is running on, and as a result it will fail. I suggest we do things the other way, instead of sizing the docker container to fit the dyno, we just use a dyno that will fit the docker container, and scale horizontally

RUN apt-get update && apt-get install -y curl # Needed for heroku
ENV PORT=$PORT
COPY ./scripts/datadog-entrypoint.sh /prod/commonwealth
RUN chmod +x /prod/commonwealth/datadog-entrypoint.sh
Copy link
Contributor

Choose a reason for hiding this comment

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

Did we decide that the max old space script is still needed? If so, perhaps it could be chained here at the RUN declaration? e.g. ./get-max-old-space-size.sh && ./datadog-entrypoint.sh

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can't use the get-max-old-space-size because the docker container does not know at image build time how much memory the container has.

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 not talking about container build time, but rather container runtime, during the RUN step.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oops, for some reason I thought that was the entrypoint 😅

What I meant was the CMD step in the other dockerfiles. Nonetheless, LGTM– tweaks can be made later.

# Conflicts:
#	libs/shared/package.json
#	libs/shared/src/canvas/runtime/node.ts
#	pnpm-lock.yaml
Copy link
Contributor

@Rotorsoft Rotorsoft left a comment

Choose a reason for hiding this comment

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

LGTM, just wondering if anything changes after @timolegros finishes work merging discord bot

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deployment plan (PRs only) requires manual infrastructure changes on release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add scripts to build/manage Docker images Use Docker for Heroku deployments
4 participants