-
Notifications
You must be signed in to change notification settings - Fork 4k
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
[WIP] chore: clarify build dependencies for future hermetic builds #10039
Changes from 19 commits
4d3d48d
f526503
2bf1c29
e1f076d
d0a4339
1eaf9b2
99c1b5c
1af9d2a
ed96171
1d4f5bc
cc69d96
d770189
496661a
34b56a4
e4576b8
05067a3
3fbe588
7ee5c4e
c9f896e
e8a75fc
8fe0f06
cd05018
989da8e
9f0d6c3
98446f8
52de92d
69aed52
781b5f4
5cb1200
704e65e
6341603
0dae350
a2da71d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -39,6 +39,7 @@ yarn-error.log | |
|
||
# Parcel default cache directory | ||
.parcel-cache | ||
nozem.json | ||
|
||
# Cloud9 | ||
.c9 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
const baseConfig = require('cdk-build-tools/config/eslintrc'); | ||
baseConfig.parserOptions.project = __dirname + '/tsconfig.json'; | ||
module.exports = baseConfig; |
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,5 +14,5 @@ nyc.config.js | |
.LAST_PACKAGE | ||
*.snk | ||
!.eslintrc.js | ||
|
||
junit.xml | ||
!test/lambda/*/*.js | ||
junit.xml |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,2 +1,2 @@ | ||
const baseConfig = require('../../../tools/cdk-build-tools/config/jest.config'); | ||
const baseConfig = require('cdk-build-tools/config/jest.config'); | ||
module.exports = baseConfig; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,14 +7,23 @@ class TestStack extends Stack { | |
constructor(scope: Construct, id: string, props?: StackProps) { | ||
super(scope, id, props); | ||
|
||
const projectRoot = process.env.NZM_PACKAGE_SOURCE | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Feels odd that we will need to do that everywhere There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's the least odd part about it. If we need to do it once we need to do it everywhere :) |
||
? path.join(process.env.NZM_PACKAGE_SOURCE, '..', '..', '..') | ||
: undefined; | ||
const testRoot = process.env.NZM_PACKAGE_SOURCE | ||
? path.join(process.env.NZM_PACKAGE_SOURCE, 'test') | ||
: __dirname; | ||
|
||
new lambda.NodejsFunction(this, 'ts-handler', { | ||
entry: path.join(__dirname, 'integ-handlers/ts-handler.ts'), | ||
projectRoot, | ||
entry: path.join(testRoot, 'integ-handlers/ts-handler.ts'), | ||
runtime: Runtime.NODEJS_12_X, | ||
minify: true, | ||
}); | ||
|
||
new lambda.NodejsFunction(this, 'js-handler', { | ||
entry: path.join(__dirname, 'integ-handlers/js-handler.js'), | ||
projectRoot, | ||
entry: path.join(testRoot, 'integ-handlers/js-handler.js'), | ||
runtime: Runtime.NODEJS_12_X, | ||
}); | ||
} | ||
|
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.
Is there a way to avoid taking this dependency on the nozem
Environment?
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 don't think so. I'll be honest with you, the
aws-lambda-nodejs
package is the bane of my existence here.In this case, this exists to tell it where the
.git
directory of the source repository is. I don't know why it needs to know that, but right now I'm just telling it in an attempt to get the build to pass.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 lookup the project root because this is what we mount as
/asset-input
to be sure to have access to all node modules and dependencies when bundling in the container. Would a fallback on looking for a lockfile (yarn.lock
,package-lock.json
) improve things here?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 feel auto discovery of the project root is a source of confusion and odd issues for many users. Maybe we should just make that explicitly required?
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 think it's nice to have it auto discovered.
What about this?
here
aws-cdk/packages/@aws-cdk/aws-lambda-nodejs/lib/bundling.ts
Lines 117 to 121 in 9ffb268
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.
Using a lockfile in this case also doesn't help me. I'm trying to do an isolated build of the package. There's a
yarn.lock
somewhere but you shouldn't care about that.My build script has set up the build environment exactly as Node-based tools should expect it (
node_modules
in the right place and everything). There's noyarn.lock
orpackage-lock.json
in sight.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.
But there's a
package.json
right? so it should work.