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

Fix lambda-sqs-worker-cdk template #695

Merged
merged 6 commits into from
Nov 26, 2021
Merged

Conversation

alazyzombie
Copy link
Contributor

@alazyzombie alazyzombie commented Nov 25, 2021

Did these things in https://github.com/SEEK-Jobs/asia-jobs-data-migrator in order to get it working:

  1. Fixed the yarn test step in buildkite (it passes locally)

  2. Fixed errors where node_modules weren't being included in the lambda

  3. fixed the weird logs in cicd on deploying for cdk
    image

@changeset-bot
Copy link

changeset-bot bot commented Nov 25, 2021

🦋 Changeset detected

Latest commit: 248e13f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
skuba Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@@ -56,12 +56,13 @@ steps:
label: 🧪 Test, Lint & Build
artifact_paths: lib/**/*
commands:
- yarn package
Copy link
Contributor Author

Choose a reason for hiding this comment

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

doesn't feel right to have to run yarn package here when it's already in Dockerfile

Copy link
Contributor

Choose a reason for hiding this comment

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

Does your test step require you to run yarn build beforehand?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I see what might be happening here. Do we need to mount lib/node_modules as a volume too? @72636c

Copy link
Contributor

@samchungy samchungy Nov 25, 2021

Choose a reason for hiding this comment

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

Could you try something for me - could you add
- /workdir/lib/node_modules to the volumes section in your docker-compose file and undo all changes except the deploy script change in your package.json in your repo?

Copy link
Member

Choose a reason for hiding this comment

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

💡 That sounds right - it looks like lib/node_modules from yarn package is never mounted.

TBH I'm questioning why yarn package is even in the Dockerfile, it sounds like it belongs on the deploy step. I believe that was the rationale behind it being --offline, ideally it would be runnable without requiring access to the npm read token.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ooo that might be due to yarn cache clean afaik we need the cache to run any with --offline

Copy link
Member

Choose a reason for hiding this comment

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

Curious, is the command doing anything that cp -R doesn't?

Copy link
Contributor

@samchungy samchungy Nov 26, 2021

Choose a reason for hiding this comment

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

Not sure aha, I think it may be possibly installing production node_modules in lib whilst leaving the Dev deps in tact? That's my theory. Running --production I think strips dev deps out yeah?

Copy link
Member

Choose a reason for hiding this comment

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

Ah of course, thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this worked, thanks!

@alazyzombie alazyzombie changed the title Update package.json Fix lambda-sqs-worker-cdk template Nov 25, 2021
@alazyzombie alazyzombie marked this pull request as ready for review November 25, 2021 06:00
@alazyzombie alazyzombie requested a review from a team as a code owner November 25, 2021 06:00
Lydia and others added 2 commits November 26, 2021 14:10
Copy link
Member

@72636c 72636c left a comment

Choose a reason for hiding this comment

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

🙇 Thanks a bunch!

.changeset/stupid-pets-explain.md Outdated Show resolved Hide resolved
@72636c 72636c enabled auto-merge (squash) November 26, 2021 04:09
@72636c 72636c merged commit ac21029 into seek-oss:master Nov 26, 2021
@github-actions github-actions bot mentioned this pull request Nov 26, 2021
@alazyzombie alazyzombie deleted the patch-1 branch November 26, 2021 04:40
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.

3 participants