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

adds docker-compose #31

Open
wants to merge 9 commits into
base: staging
Choose a base branch
from
Open

adds docker-compose #31

wants to merge 9 commits into from

Conversation

AlanBreck
Copy link
Collaborator

@AlanBreck AlanBreck commented Mar 27, 2023

This adds docker-compose for easier local development set up.

Resolves https://github.com/brave/devops/issues/9555

@AlanBreck AlanBreck self-assigned this Mar 27, 2023
@AlanBreck AlanBreck requested a review from kim0 March 27, 2023 23:36
docker-compose.yml Show resolved Hide resolved
docker-compose.yml Show resolved Hide resolved
docker-compose.yml Show resolved Hide resolved
docker-compose.yml Show resolved Hide resolved
docker-compose.yml Show resolved Hide resolved
docker-compose.yml Show resolved Hide resolved
docker-compose.yml Show resolved Hide resolved
docker-compose.yml Show resolved Hide resolved
docker-compose.yml Show resolved Hide resolved
docker-compose.yml Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@socket-security
Copy link

socket-security bot commented Mar 27, 2023

👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎

This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored.

@thypon
Copy link
Member

thypon commented Mar 28, 2023

@AlanBreck May you add the advised docker-compose flags?

docker-compose.yml Outdated Show resolved Hide resolved
docker-compose.yml Outdated Show resolved Hide resolved
kim0
kim0 previously approved these changes Mar 28, 2023
Copy link
Contributor

@kim0 kim0 left a comment

Choose a reason for hiding this comment

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

LGTM

@AlanBreck
Copy link
Collaborator Author

@AlanBreck May you add the advised docker-compose flags?

@kim0 kindly added those flags. It appears that the comments didn't resolve themselves, though. @thypon, can you verify that everything looks good?

@bcaller
Copy link

bcaller commented Mar 29, 2023

Yes, the flags look fine. I can't remove the needs-security-review label myself.

kim0
kim0 previously approved these changes Mar 29, 2023
@kim0
Copy link
Contributor

kim0 commented Mar 29, 2023

I could remove the label and resolved the bot conversations.
@AlanBreck it's worth it to re-test after those flags were added just to make sure they don't break things.

@AlanBreck
Copy link
Collaborator Author

Good call @kim0! It does in fact break.

@AlanBreck
Copy link
Collaborator Author

@kim0, would you mind taking a peak at this if/when you get a chance? Not high priority, but I'm not quite sure how to proceed.

@kim0
Copy link
Contributor

kim0 commented Apr 18, 2023

@AlanBreck Sure. Took a shot today, hit the below. How can I get past this

 > [7/7] RUN npm run build:
#0 1.603
#0 1.603 > [email protected] build
#0 1.603 > vite build
#0 1.603
#0 1.922 ▲ [WARNING] Cannot find base config file "./.svelte-kit/tsconfig.json" [tsconfig.json]
#0 1.922
#0 1.922     jsconfig.json:2:13:
#0 1.922       2 │   "extends": "./.svelte-kit/tsconfig.json",
#0 1.922         ╵              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
#0 1.922
#0 3.013 error during build:
#0 3.013 Error: config.kit.csp.directives.img-src must be an array of strings, if specified
#0 3.013     at file:///app/node_modules/@sveltejs/kit/src/core/config/options.js:328:10
#0 3.013     at file:///app/node_modules/@sveltejs/kit/src/core/config/options.js:300:43
#0 3.013     at file:///app/node_modules/@sveltejs/kit/src/core/config/options.js:286:18
#0 3.013     at file:///app/node_modules/@sveltejs/kit/src/core/config/options.js:286:18
#0 3.013     at file:///app/node_modules/@sveltejs/kit/src/core/config/options.js:286:18
#0 3.013     at file:///app/node_modules/@sveltejs/kit/src/core/config/options.js:286:18
#0 3.013     at validate_config (file:///app/node_modules/@sveltejs/kit/src/core/config/index.js:108:9)
#0 3.013     at process_config (file:///app/node_modules/@sveltejs/kit/src/core/config/index.js:80:20)
#0 3.013     at load_config (file:///app/node_modules/@sveltejs/kit/src/core/config/index.js:72:9)
#0 3.013     at async sveltekit (file:///app/node_modules/@sveltejs/kit/src/exports/vite/index.js:116:24)
------
failed to solve: executor failed running [/bin/sh -c npm run build]: exit code: 1

Also I might be missing .env files.

@AlanBreck
Copy link
Collaborator Author

@kim0, sorry, try this:

PUBLIC_ASSETS_PATH=https://cdn.store.bravesoftware.com npm run dev

@kim0
Copy link
Contributor

kim0 commented May 11, 2023

@AlanBreck spent some cycles on this. It seems to "start" correctly now. I'm able to hit the api endpoint correctly, however store/vite is issuing an error which is probably easy for you to fix. Might also be the branch being old, and needing a sync from main

store                                   | [1] error when starting dev server:
store                                   | [1] Error: config.kit.csp.directives.img-src must be an array of strings, if specified

The main changes I just pushed are:

* Running as user `node` which is recommended by the node image
* Not using a customized Dockerfile, instead trying to inject any needed changes from the compose file
* store and api get around 1.5GB of reserved RAM.
* Every writeable location gets its own "volume" to allow for the read-only fs setting
* Overriding the Dockerfile builtin command, and using a dev cycle friendly command
* The code is mounted (live) to the docker image, which works well for a fast dev cycle (no need to rebuild image repeatedly)

Please test and let me know how it goes!

@socket-security
Copy link

New dependencies detected. Learn more about Socket for GitHub ↗︎

Packages Version New capabilities Transitives Size Publisher
concurrently 7.6.0 None +2 131 kB gustavohenke

@mihaiplesa
Copy link
Contributor

mihaiplesa commented Aug 29, 2023

Just rebased to fix conflicts but still needs some attention from @AlanBreck.

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

Successfully merging this pull request may close these issues.

5 participants