-
Notifications
You must be signed in to change notification settings - Fork 21
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
Convert cron, ceramic and websockets into monorepo apps #4556
Conversation
key: webapp-${{ hashFiles('**/package-lock.json') }}-${{ hashFiles('**/*.ts', '**/*.tsx', '**/*.gql', '**/*.scss', '!node_modules', '!.next', '!dist', '!apps') }} | ||
restore-keys: | | ||
webapp-${{ hashFiles('**/package-lock.json') }}- | ||
|
||
- name: Build | ||
shell: bash | ||
if: steps.restore_build.outputs.cache-hit != 'true' | ||
run: npm run build |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we not going to save the build cache anymore 😮
.github/actions/build_app/action.yml
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we have two separate build actions? Can't we use this one for building the webapp too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the webapp should eventually use build_app but first we should make it a workspace app
@@ -39,13 +43,14 @@ runs: | |||
path: | | |||
./node_modules | |||
./apps/*/node_modules |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we only include the node_modules in the correct app rather than ./apps/*/node_modules
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
they won't be built so there's nothing to be copied
AWS_SECRET_ACCESS_KEY: ${{ secrets.AWS_SECRET_ACCESS_KEY }} | ||
AWS_REGION: us-east-1 | ||
with: | ||
ecr_registry: cron |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that we are using a separate registry for cron. I believe previously we were using the webapp one
@@ -138,7 +136,7 @@ jobs: | |||
file_extensions: '**/*.{ts,tsx}' | |||
|
|||
- name: Run tests | |||
run: npm run test -w apps/scoutgame |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When testing cron we are using the -w apps/cron format. But for scoutgame and I assume other non webapp apps we are using app_name:test
format. Shouldn't we stick to a single format?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
|
||
- name: Update Dockerfile | ||
run: | | ||
rm Dockerfile && mv apps/websockets/Dockerfile Dockerfile |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love that now each app have their own ecr registry along with Dockerfile
"typecheck": "../../node_modules/typescript/bin/tsc --project ./tsconfig.json --noEmit" | ||
}, | ||
"dependencies": { | ||
"@packages/charmeditor": "^1.0.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice I didn't know you could do that 😮
prd-sockets: | ||
extends: | ||
service: defaults | ||
ports: | ||
- '80:3000' | ||
command: ['sh', '-c', 'node ./dist/initWebsockets.js'] | ||
command: ['sh', '-c', 'node ./apps/websockets/dist/initWebsockets.js'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we use an npm command like npm run websockets:start:prod
or something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we're using node directly in this file. there is a difference when you use npm, i think it changes how the process gets run inside of docker but unfortunately i don't recall the issue we were addressing anymore. so for now it's also to be consistent
Tested: