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

feat: Allow developers to run API in docker, and select between mutable and immutable modes #1888

Open
wants to merge 194 commits into
base: main
Choose a base branch
from

Conversation

redaphid
Copy link
Contributor

@redaphid redaphid commented May 3, 2022

This PR intends to simplify the development ergonomics of nft.storage. It does so by simplifying the environment via docker-compose and uses the environment variables passed into docker-compose as the only source of truth for configuration.

  • Any developer on the project can simply copy .env.tpl to .env, then run yarn dev:api to start the api system up in an immutable state.
  • For development work, the .env file defines the constants for playwright, the cloudflare worker, and Github Actions CI.
  • Github Actions now runs the same command as a developer starting the system. (e.g. copying .env.tpl to .env and running yarn test)
  • yarn test can be run at the same time as the development cluster without port conflicts or data cross-contamination.
  • yarn test:watch in api watches for code changes, then re-runs the playwright tests in an isolated environment on save.
  • yarn dev watches the code, and on save rebuilds
  • yarn dev:persist persists the database in a docker volume between runs.
  • yarn dev:clean will clean the database state for yarn dev:persist
  • yarn test will run playwright locally against the docker-compose cluster
  • yarn test:docker will run playwright itself inside a docker container`

redaphid added 30 commits April 11, 2022 11:41
yusefnapora and others added 6 commits June 2, 2022 17:51
…exit code, as this was causing problems in the 'persist' mode. Added a `yarn dev:clean` to clean the volumes for the persist mode. Managed to remove one docker-compose file :)
@redaphid
Copy link
Contributor Author

redaphid commented Jun 2, 2022

@hugomrdias We incorporated your feedback :). Mind looking this over again when you have the chance?

@redaphid redaphid requested a review from vasco-santos June 7, 2022 18:01
Copy link
Contributor

@vasco-santos vasco-santos left a comment

Choose a reason for hiding this comment

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

See that we had a lot of back and forth here. Apologies if I added some questions in comments that were already discussed :)

I like the overall direction here, I like to have both options of using docker and local environment for testing. Left some minor suggestions so that we can get this merged in

.env.tpl Outdated
@@ -1,38 +1,40 @@
# needs to be real so create a personal magic.link account or use the staging key
MAGIC_SECRET_KEY=secret

Copy link
Contributor

Choose a reason for hiding this comment

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

Could we keep same comments as before?

## PostgREST
# API Secrets

I find them useful when we need to tweak the environment, for instance if we need to do. debug session in staging environment

apiToken: ${{secrets.CF_API_TOKEN }}
workingDirectory: 'packages/api'
- run: 'cp .env.tpl .env'
- run: yarn test:api:docker
Copy link
Contributor

Choose a reason for hiding this comment

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

I like the cp approach here to better simulate what we do.

Not sure I understand why we are getting rid of deploy-dev? In web3.storage we were feeling we should have it too, as this is useful to test something without merging to main, as other things might need to be shipped and get blocked on a staging test for a large feature.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's possible that @redaphid and I were confused about deploy-dev. We both thought it was a remnant of the local-tunnel days, where you had to deploy to cloudflare in order to run the dev environment at all.

If it's still useful as an ad-hoc "not quite staging" env we can bring it back.

Copy link
Contributor

Choose a reason for hiding this comment

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

If it's still useful as an ad-hoc "not quite staging" env we can bring it back.

It is, but it will be re-written with new commits in other PRs which is annoying. Would love to be able to trigger it in CI for a specific branch, but I think we won't be able to provide a branch on a manual workflow trigger

Copy link
Contributor

Choose a reason for hiding this comment

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

yes please keep the deploy-dev job and also this yaml is not valid at all i think.

services:
rest:
post-rest:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
post-rest:
postgrest:

build:
dockerfile: './docker/Dockerfile'
context: '../'
entrypoint: 'psql $DATABASE_CONNECTION -v "ON_ERROR_STOP=1" -f ./db/config.sql -f ./db/tables.sql -f ./db/cargo.testing.sql -f ./db/functions.sql'
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should have a init.sql script here. If we add a new sql file that needs to run, it will be difficult for the developer to understand the multiple places where it should be added. So, having a init.sql that would execute the other files would be rad. Not super sure how to do it, but found this

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a good idea, especially if we start having fancier migrations - we can just hook them into the init script.

@@ -1,13 +1,7 @@
FROM supabase/postgres:13.3.0
FROM supabase/postgres:12.5.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we downgrading?

packages/api/pw-test.config.cjs Show resolved Hide resolved
Comment on lines 14 to 15
"test": "./scripts/start-test-local.sh",
"test:docker": "./scripts/start-test.sh ",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"test": "./scripts/start-test-local.sh",
"test:docker": "./scripts/start-test.sh ",
"test": "./scripts/start-test.sh",
"test:docker": "./scripts/start-test-docker.sh ",

Can we keep same namings to avoid confusion? We would need shell script names renamed

@redaphid
Copy link
Contributor Author

@vasco-santos I've incorporated all your suggestions! Lmk if this works for you.

Copy link
Contributor

@hugomrdias hugomrdias left a comment

Choose a reason for hiding this comment

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

Can we please remove all the playwright and miniflare out of docker and focus on the DB.
Also please avoid any formatting not enforced by the repo config to make PRs simpler to review.

@@ -1,14 +1,8 @@
FROM supabase/postgres:13.3.0
FROM supabase/postgres:13.3.0

COPY 00-initial-schema.sql /docker-entrypoint-initdb.d/00-initial-schema.sql
Copy link
Contributor

Choose a reason for hiding this comment

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

we should add the other sql files here

apiToken: ${{secrets.CF_API_TOKEN }}
workingDirectory: 'packages/api'
- run: 'cp .env.tpl .env'
- run: yarn test:api:docker
Copy link
Contributor

Choose a reason for hiding this comment

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

yes please keep the deploy-dev job and also this yaml is not valid at all i think.

Copy link
Contributor

@vasco-santos vasco-santos left a comment

Choose a reason for hiding this comment

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

We should update this PR to:

  1. Scope only development setup, let's have test setup from chore: convert tests to use miniflare + ava #2065
  2. In the API package we are building the API, so let's not dockerize the API (either Miniflare or Playwright) and dockerize only external services, i.e Cluster, DB, ...

@@ -26,7 +26,7 @@ eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiJkaWQ6ZXRocjoweDY1MDA3QTczOWFiN0F
In the case you need to clean up docker after failed tests or debugging session you can just run the command below.

```bash
yarn clean
yarn dev:clean
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 have a dev:clean script in api package

Comment on lines +93 to +126
playwright:
profiles:
- test
depends_on:
api:
condition: service_healthy
build:
dockerfile: ./docker/test.Dockerfile
context: ../
environment:
- BRANCH
- CLUSTER_API_URL
- CLUSTER_BASIC_AUTH_TOKEN
- CLUSTER_SERVICE
- COMMITHASH
- DATABASE_CONNECTION
- DATABASE_TOKEN
- DATABASE_URL
- DEBUG
- ENV
- LOGTAIL_TOKEN
- MAGIC_SECRET_KEY
- MAILCHIMP_API_KEY
- MAINTENANCE_MODE
- METAPLEX_AUTH_TOKEN
- PRIVATE_KEY
- S3_ACCESS_KEY_ID
- S3_BUCKET_NAME
- S3_ENDPOINT
- S3_REGION
- S3_SECRET_ACCESS_KEY
- SALT
- SENTRY_DSN
- VERSION
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
playwright:
profiles:
- test
depends_on:
api:
condition: service_healthy
build:
dockerfile: ./docker/test.Dockerfile
context: ../
environment:
- BRANCH
- CLUSTER_API_URL
- CLUSTER_BASIC_AUTH_TOKEN
- CLUSTER_SERVICE
- COMMITHASH
- DATABASE_CONNECTION
- DATABASE_TOKEN
- DATABASE_URL
- DEBUG
- ENV
- LOGTAIL_TOKEN
- MAGIC_SECRET_KEY
- MAILCHIMP_API_KEY
- MAINTENANCE_MODE
- METAPLEX_AUTH_TOKEN
- PRIVATE_KEY
- S3_ACCESS_KEY_ID
- S3_BUCKET_NAME
- S3_ENDPOINT
- S3_REGION
- S3_SECRET_ACCESS_KEY
- SALT
- SENTRY_DSN
- VERSION

"dev:website": "cd packages/website && yarn dev",
"test": "run-s test:*",
"test:client": "yarn --cwd packages/client test",
"test:api": "yarn --cwd packages/api test",
"test:api:docker": "yarn --cwd packages/api test:docker",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"test:api:docker": "yarn --cwd packages/api test:docker",

Comment on lines +1 to +20
FROM mcr.microsoft.com/playwright:v1.20.0-focal

RUN npm i -g nodemon typescript
RUN mkdir -p /app

WORKDIR /app

# Make your docker builds 100x faster with this one trick :)
COPY ./package.json .
COPY ./tsconfig.json .
RUN yarn install

COPY ./pw-test.config.cjs .
COPY ./docker/scripts ./docker-scripts
COPY ./scripts ./scripts
COPY ./db ./db
COPY ./src/ ./src
COPY ./test/ ./test

ENTRYPOINT ["./docker-scripts/run-playwright.sh" ]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
FROM mcr.microsoft.com/playwright:v1.20.0-focal
RUN npm i -g nodemon typescript
RUN mkdir -p /app
WORKDIR /app
# Make your docker builds 100x faster with this one trick :)
COPY ./package.json .
COPY ./tsconfig.json .
RUN yarn install
COPY ./pw-test.config.cjs .
COPY ./docker/scripts ./docker-scripts
COPY ./scripts ./scripts
COPY ./db ./db
COPY ./src/ ./src
COPY ./test/ ./test
ENTRYPOINT ["./docker-scripts/run-playwright.sh" ]

Comment on lines +14 to +15
"test": "./scripts/start-test.sh",
"test:docker": "./scripts/start-test-docker.sh ",
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's keep test as it was and move forward with tests like #2065

Comment on lines +1 to +37
#!/usr/bin/env bash
set -eX

# We're moving this script around a lot, and it's pretty cwd-dependent.
ENV_FILE=../../.env
COMPOSE_FILES="--file ./docker/docker-compose.yml --file ./docker/docker-compose.local-ports.yml"

cleanup() {
docker compose --project-name "nft-storage-api-local-tests" down --remove-orphans
}
trap cleanup EXIT

docker compose \
--project-name="nft-storage-api-local-tests" \
$COMPOSE_FILES --env-file="$ENV_FILE" up \
--detach \
--always-recreate-deps \
--remove-orphans \
--force-recreate \
--renew-anon-volumes \
--build \
;

THIS_DIR="$( cd -- "$(dirname "$0")" >/dev/null 2>&1 ; pwd -P )"
API_DIR="$THIS_DIR/../"

pushd $API_DIR

export DATABASE_URL="http://localhost:3000"
export CLUSTER_API_URL="http://localhost:9094"

tsc \
&& npx playwright-test "./test/"'**/*.spec.js' --sw src/index.js \
;


popd
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#!/usr/bin/env bash
set -eX
# We're moving this script around a lot, and it's pretty cwd-dependent.
ENV_FILE=../../.env
COMPOSE_FILES="--file ./docker/docker-compose.yml --file ./docker/docker-compose.local-ports.yml"
cleanup() {
docker compose --project-name "nft-storage-api-local-tests" down --remove-orphans
}
trap cleanup EXIT
docker compose \
--project-name="nft-storage-api-local-tests" \
$COMPOSE_FILES --env-file="$ENV_FILE" up \
--detach \
--always-recreate-deps \
--remove-orphans \
--force-recreate \
--renew-anon-volumes \
--build \
;
THIS_DIR="$( cd -- "$(dirname "$0")" >/dev/null 2>&1 ; pwd -P )"
API_DIR="$THIS_DIR/../"
pushd $API_DIR
export DATABASE_URL="http://localhost:3000"
export CLUSTER_API_URL="http://localhost:9094"
tsc \
&& npx playwright-test "./test/"'**/*.spec.js' --sw src/index.js \
;
popd

Comment on lines +1 to +17
#!/usr/bin/env sh

ENV_FILE=../../.env
docker compose --profile="test" --file ./docker/docker-compose.yml --env-file="$ENV_FILE" config

docker compose \
--project-name="nft-storage-api-test" \
--profile="test" --file ./docker/docker-compose.yml --env-file="$ENV_FILE" up \
--always-recreate-deps \
--force-recreate \
--renew-anon-volumes \
--remove-orphans \
--build \
--attach "playwright" \
--no-log-prefix \
;

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#!/usr/bin/env sh
ENV_FILE=../../.env
docker compose --profile="test" --file ./docker/docker-compose.yml --env-file="$ENV_FILE" config
docker compose \
--project-name="nft-storage-api-test" \
--profile="test" --file ./docker/docker-compose.yml --env-file="$ENV_FILE" up \
--always-recreate-deps \
--force-recreate \
--renew-anon-volumes \
--remove-orphans \
--build \
--attach "playwright" \
--no-log-prefix \
;

Comment on lines +1 to +26
#!/usr/bin/env bash

npx miniflare --debug \
--build-command "npm run build" \
--binding ENV="$ENV" \
--binding PRIVATE_KEY="$PRIVATE_KEY" \
--binding DATABASE_CONNECTION="$DATABASE_CONNECTION" \
--binding DATABASE_TOKEN="$DATABASE_TOKEN" \
--binding DATABASE_URL="$DATABASE_URL" \
--binding LOGTAIL_TOKEN="$LOGTAIL_TOKEN" \
--binding MAGIC_SECRET_KEY="$MAGIC_SECRET_KEY" \
--binding SALT="$SALT" \
--binding SENTRY_DSN="$SENTRY_DSN" \
--binding MAINTENANCE_MODE="$MAINTENANCE_MODE" \
--binding MAILCHIMP_API_KEY="$MAILCHIMP_API_KEY" \
--binding CLUSTER_API_URL="$CLUSTER_API_URL" \
--binding CLUSTER_BASIC_AUTH_TOKEN="$CLUSTER_BASIC_AUTH_TOKEN" \
--binding DEBUG="$DEBUG" \
--binding METAPLEX_AUTH_TOKEN="$METAPLEX_AUTH_TOKEN" \
--binding S3_ACCESS_KEY_ID="$S3_ACCESS_KEY_ID" \
--binding S3_BUCKET_NAME="$S3_BUCKET_NAME" \
--binding S3_ENDPOINT="$S3_ENDPOINT" \
--binding S3_REGION="$S3_REGION" \
--binding S3_SECRET_ACCESS_KEY="$S3_SECRET_ACCESS_KEY" \
;

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#!/usr/bin/env bash
npx miniflare --debug \
--build-command "npm run build" \
--binding ENV="$ENV" \
--binding PRIVATE_KEY="$PRIVATE_KEY" \
--binding DATABASE_CONNECTION="$DATABASE_CONNECTION" \
--binding DATABASE_TOKEN="$DATABASE_TOKEN" \
--binding DATABASE_URL="$DATABASE_URL" \
--binding LOGTAIL_TOKEN="$LOGTAIL_TOKEN" \
--binding MAGIC_SECRET_KEY="$MAGIC_SECRET_KEY" \
--binding SALT="$SALT" \
--binding SENTRY_DSN="$SENTRY_DSN" \
--binding MAINTENANCE_MODE="$MAINTENANCE_MODE" \
--binding MAILCHIMP_API_KEY="$MAILCHIMP_API_KEY" \
--binding CLUSTER_API_URL="$CLUSTER_API_URL" \
--binding CLUSTER_BASIC_AUTH_TOKEN="$CLUSTER_BASIC_AUTH_TOKEN" \
--binding DEBUG="$DEBUG" \
--binding METAPLEX_AUTH_TOKEN="$METAPLEX_AUTH_TOKEN" \
--binding S3_ACCESS_KEY_ID="$S3_ACCESS_KEY_ID" \
--binding S3_BUCKET_NAME="$S3_BUCKET_NAME" \
--binding S3_ENDPOINT="$S3_ENDPOINT" \
--binding S3_REGION="$S3_REGION" \
--binding S3_SECRET_ACCESS_KEY="$S3_SECRET_ACCESS_KEY" \
;

Comment on lines +1 to +5
#!/usr/bin/env bash

tsc \
&& npx playwright-test 'test/**/*.spec.js' --sw src/index.js \
;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#!/usr/bin/env bash
tsc \
&& npx playwright-test 'test/**/*.spec.js' --sw src/index.js \
;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants