-
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
feat(codebuild): Lambda compute for codebuild projects #27934
Conversation
…add-lambda-compute-suport
packages/aws-cdk-lib/aws-codebuild/test/linux-arm-lambda-build-image.test.ts
Show resolved
Hide resolved
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.
Thanks for the contribution! 👍
Looks good overall, just some notes on documentation and code structure.
* @see https://docs.aws.amazon.com/codebuild/latest/userguide/build-env-ref-available.html | ||
*/ | ||
export class LinuxArmLambdaBuildImage implements IBuildImage { | ||
/** Image "aws/codebuild/amazonlinux-aarch64-lambda-standard:nodejs18". */ |
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.
/** Image "aws/codebuild/amazonlinux-aarch64-lambda-standard:nodejs18". */ | |
/** The `aws/codebuild/amazonlinux-aarch64-lambda-standard:nodejs18` build image. */ |
Let's keep this format.
* This class has a bunch of public constants that represent the CodeBuild aarch64 images. | ||
* |
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.
* This class has a bunch of public constants that represent the CodeBuild aarch64 images. | |
* | |
* This class has a bunch of public constants that represent the CodeBuild aarch64 Lambda images. |
/** | ||
* Validates by checking unsupported property and compute type | ||
* @param buildEnvironment BuildEnvironment | ||
*/ |
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.
/** | |
* Validates by checking unsupported property and compute type | |
* @param buildEnvironment BuildEnvironment | |
*/ |
Let's keep the interface documentation.
const ret = []; | ||
|
||
if (buildEnvironment.privileged) { | ||
ret.push('Lambda compute type does not support Privileged mode'); |
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.
ret.push('Lambda compute type does not support Privileged mode'); | |
ret.push('Lambda compute type does not support privileged mode'); |
Since we are here, can you please also add the validation in WindowsBuildImage
? (see last note)
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.
Same validation and testing added for WindowsBuildImage
.
* This class has a bunch of public constants that represent the CodeBuild x86-64 images. | ||
* |
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.
* This class has a bunch of public constants that represent the CodeBuild x86-64 images. | |
* | |
* This class has a bunch of public constants that represent the CodeBuild Lambda x86-64 images. |
// Lambda compute does not support timeoutInMinutes property | ||
if (this.isLambdaComputeType(this.buildImage)) { | ||
if (props.timeout) { | ||
errors.push('Cannot specify timeoutInMinutes for lambda compute'); | ||
} | ||
if (props.queuedTimeout) { | ||
errors.push('Cannot specify queuedTimeoutInMinutes for lambda compute'); | ||
} | ||
if (props.cache) { | ||
errors.push('Cannot specify cache for lambda compute'); | ||
} | ||
if (props.badge) { | ||
errors.push('Cannot specify badgeEnabled for lambda compute'); | ||
} | ||
} |
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.
// Lambda compute does not support timeoutInMinutes property | |
if (this.isLambdaComputeType(this.buildImage)) { | |
if (props.timeout) { | |
errors.push('Cannot specify timeoutInMinutes for lambda compute'); | |
} | |
if (props.queuedTimeout) { | |
errors.push('Cannot specify queuedTimeoutInMinutes for lambda compute'); | |
} | |
if (props.cache) { | |
errors.push('Cannot specify cache for lambda compute'); | |
} | |
if (props.badge) { | |
errors.push('Cannot specify badgeEnabled for lambda compute'); | |
} | |
} | |
errors.push(...validateLambdaBuildImage(this.buildImage, this.props)); |
Can you please declare a separate function for this?
/**
* Validates a Lambda build image given the project properties.
* @see https://docs.aws.amazon.com/codebuild/latest/userguide/lambda.html#lambda.limitations
*/
private validateLambdaBuildImage(buildImage: IBuildImage, props: ProjectProps): string[] {
if (!isLambdaBuildImage(buildImage)) return [];
const ret = [];
if (props.timeout) {
ret.push('Cannot specify timeoutInMinutes for lambda compute');
}
if (props.queuedTimeout) {
ret.push('Cannot specify queuedTimeoutInMinutes for lambda compute');
}
if (props.cache) {
ret.push('Cannot specify cache for lambda compute');
}
if (props.badge) {
ret.push('Cannot specify badgeEnabled for lambda compute');
}
return ret;
}
const ret = []; | ||
|
||
const lambdaComputeTypes = Object.values(ComputeType).filter(value => value.startsWith('BUILD_LAMBDA')); | ||
if (_env.computeType && lambdaComputeTypes.includes(_env.computeType)) { | ||
ret.push(`x86-64 images only support ComputeTypes between '${ComputeType.SMALL}' and '${ComputeType.X2_LARGE}' - ` + | ||
`'${_env.computeType}' was given`); | ||
} | ||
|
||
return ret; |
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.
const ret = []; | |
const lambdaComputeTypes = Object.values(ComputeType).filter(value => value.startsWith('BUILD_LAMBDA')); | |
if (_env.computeType && lambdaComputeTypes.includes(_env.computeType)) { | |
ret.push(`x86-64 images only support ComputeTypes between '${ComputeType.SMALL}' and '${ComputeType.X2_LARGE}' - ` + | |
`'${_env.computeType}' was given`); | |
} | |
return ret; | |
const ret = []; | |
const lambdaComputeTypes = Object.values(ComputeType).filter(value => value.startsWith('BUILD_LAMBDA')); | |
if (this.isLambdaComputeType(env.computeType)) { | |
ret.push('x86-64 images do not support Lambda compute mode'); | |
} | |
return ret; |
It's better to declare a separate function to check if the compute type is a Lambda one and re-use it.
private isLambdaComputeType(computeType?: string): boolean {
if (!computeType) return false;
return [
ComputeType.LAMBDA_1GB,
ComputeType.LAMBDA_2GB,
...
].includes(computeType);
}
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.
added implementation as a function instead of a method so that it can be used in other files.
@@ -1542,6 +1562,10 @@ export class Project extends ProjectBase { | |||
throw new Error('Both source and artifacts must be set to CodePipeline'); | |||
} | |||
} | |||
|
|||
private isLambdaComputeType(buildImage: IBuildImage): boolean { |
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.
private isLambdaComputeType(buildImage: IBuildImage): boolean { | |
private isLambdaBuildImage(buildImage: IBuildImage): boolean { |
Naming.
Custom images are not supported, so please use a static method such as `AMAZON_LINUX_2_NODE_18`. | ||
See documentation for other constraints. | ||
https://docs.aws.amazon.com/codebuild/latest/userguide/lambda.html#lambda.limitations |
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.
Custom images are not supported, so please use a static method such as `AMAZON_LINUX_2_NODE_18`. | |
See documentation for other constraints. | |
https://docs.aws.amazon.com/codebuild/latest/userguide/lambda.html#lambda.limitations | |
> Visit [AWS Lambda compute in AWS CodeBuild](https://docs.aws.amazon.com/codebuild/latest/userguide/lambda.html) for more details. |
? ImagePullPrincipalType.CODEBUILD | ||
: ImagePullPrincipalType.SERVICE_ROLE; | ||
// For Lambda compute, specifying imagePullPrincipalType is not supported | ||
const imagePullPrincipalType = this.isLambdaComputeType(this.buildImage) |
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 this needed?
If CFN does not deploy it should be validated in validateLambdaBuildImage
otherwise I think we can just ignore the value.
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.
In the original implementation, SERVICE_ROLE
is specified as the default value even if imagePullPrincipalType
is not specified.
const imagePullPrincipalType = this.buildImage.imagePullPrincipalType === ImagePullPrincipalType.CODEBUILD |
const imagePullPrincipalType = this.buildImage.imagePullPrincipalType === ImagePullPrincipalType.CODEBUILD
? ImagePullPrincipalType.CODEBUILD
: ImagePullPrincipalType.SERVICE_ROLE;
return {
imagePullCredentialsType: imagePullPrincipalType,
};
Therefore, I think some kind of modification other than validation is necessary.
Any advice on a better implementation would be appreciated!
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 that imagePullPrincipalType
, secretsManagerCredentials
, and repository
will just be ignored if a Lambda-compute image is passed (I might be wrong on this).
If this is the case we could provide some feedback with Annotations.addWarningV2()
to notify the users that those properties will not be used.
I don't think we should set imagePullPrincipalType
to undefined
if this.isLambdaBuildImage(this.buildImage)
because it would either unset an already ignored value or mask a wrong property specification that should be validated.
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 that imagePullPrincipalType, secretsManagerCredentials, and repository will just be ignored if a Lambda-compute image is passed (I might be wrong on this).
I agree with you.
imagePullPrincipalType
, secretsManagerCredentials
, and repository
are all specified within IBuildImage
, but the Lambda Build Image doesn't have these properties. Therefore, users generally cannot explicitly specify these values.
However, when it comes to imagePullPrincipalType
, even if these values are not specified within the Lambda Build Image, the Project.renderEnvironment
method defaults to setting imagePullPrincipalType
to SERVICE_ROLE
. This causes deployment to result in an error message Cannot specify imagePullPrincipalType for lambda compute
.
So, I think some modifications is needed in lines like the following within the renderEnvironment
method for the deployment to work correctly.
const imagePullPrincipalType = this.buildImage.imagePullPrincipalType === ImagePullPrincipalType.CODEBUILD |
imagePullCredentialsType: imagePullPrincipalType, |
@lpizzinidev |
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.
Thanks for the changes 👍
Just some minor adjustments are needed.
Also, I would change the title to feat(codebuild): support Lambda compute
.
if (isLambdaComputeType(_env.computeType)) { | ||
ret.push('x86-64 images do not support Lambda compute mode'); | ||
} |
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.
if (isLambdaComputeType(_env.computeType)) { | |
ret.push('x86-64 images do not support Lambda compute mode'); | |
} | |
if (isLambdaComputeType(env.computeType)) { | |
ret.push('x86-64 images do not support Lambda compute types'); | |
} |
Seems more appropriate, can you please also update the other similar error messages?
Also, _
should be removed from the property name as it's now in use.
import * as codebuild from '../lib'; | ||
|
||
describe('Linux ARM Lambda build image', () => { | ||
describe('AMAZON_LINUX_2_NODE_18', () => { |
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.
describe('AMAZON_LINUX_2_NODE_18', () => { |
Let's keep only the generic describe as these tests apply also to other image types.
import * as codebuild from '../lib'; | ||
|
||
describe('Linux Lambda build image', () => { | ||
describe('AMAZON_LINUX_2_NODE_18', () => { |
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.
describe('AMAZON_LINUX_2_NODE_18', () => { |
if (props.timeout) { | ||
ret.push('Cannot specify timeoutInMinutes for lambda compute'); | ||
} | ||
if (props.queuedTimeout) { | ||
ret.push('Cannot specify queuedTimeoutInMinutes for lambda compute'); | ||
} | ||
if (props.cache) { | ||
ret.push('Cannot specify cache for lambda compute'); | ||
} | ||
if (props.badge) { | ||
ret.push('Cannot specify badgeEnabled for lambda compute'); | ||
} |
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.
if (props.timeout) { | |
ret.push('Cannot specify timeoutInMinutes for lambda compute'); | |
} | |
if (props.queuedTimeout) { | |
ret.push('Cannot specify queuedTimeoutInMinutes for lambda compute'); | |
} | |
if (props.cache) { | |
ret.push('Cannot specify cache for lambda compute'); | |
} | |
if (props.badge) { | |
ret.push('Cannot specify badgeEnabled for lambda compute'); | |
} | |
if (props.timeout) { | |
ret.push('Cannot specify timeout for Lambda compute'); | |
} | |
if (props.queuedTimeout) { | |
ret.push('Cannot specify queuedTimeout for Lambda compute'); | |
} | |
if (props.cache) { | |
ret.push('Cannot specify cache for Lambda compute'); | |
} | |
if (props.badge) { | |
ret.push('Cannot enable badge for Lambda compute'); | |
} |
Let's keep CDK property names.
Additionally, with the introduction of Lambda compute support, the `LambdaBuildImage` (or `LinuxArmLambdaBuildImage`) class | ||
is available for specifying Lambda-compatible images. |
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.
Additionally, with the introduction of Lambda compute support, the `LambdaBuildImage` (or `LinuxArmLambdaBuildImage`) class | |
is available for specifying Lambda-compatible images. | |
With the introduction of Lambda compute support, the `LinuxLambdaBuildImage ` (or `LinuxArmLambdaBuildImage`) class | |
is available for specifying Lambda-compatible images. |
@@ -329,6 +332,8 @@ or one of the corresponding methods on `LinuxArmBuildImage`: | |||
* `LinuxArmBuildImage.fromDockerRegistry(image[, { secretsManagerCredentials }])` | |||
* `LinuxArmBuildImage.fromEcrRepository(repo[, tag])` | |||
|
|||
However, be aware that specifying custom images is not possible with Lambda Images. |
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.
However, be aware that specifying custom images is not possible with Lambda Images. | |
**Note:** You cannot specify custom images on `LinuxLambdaBuildImage` or `LinuxArmLambdaBuildImage` images. |
// eslint-disable-next-line no-duplicate-imports, import/order | ||
/* eslint-disable no-duplicate-imports, import/order */ | ||
import { LinuxArmBuildImage } from './linux-arm-build-image'; | ||
import { LinuxArmLambdaBuildImage } from './linux-arm-lambda-build-image'; | ||
import { LinuxLambdaBuildImage } from './linux-lambda-build-image'; | ||
/* eslint-enable no-duplicate-imports, import/order */ |
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 new imports should be added at the top of the file as not directly related to LinuxBuildImage
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.
Since LinuxArmLambdaBuildImage
and LinuxLambdaBuildImage
reference ComputeType
, it's necessary to place the import statements below ComputeType
. I've adjusted their placement just beneath ComputeType
. How does that look?
However, these classes only use ComputeType
when generating error messages. It's also possible to move the import statements to the top of the file by modifying the error messages that don't use ComputeType
. This option might be more appropriate.
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.
Thanks for clarifying, let's just keep them together as you did in the original commit then. It's clearer.
? ImagePullPrincipalType.CODEBUILD | ||
: ImagePullPrincipalType.SERVICE_ROLE; | ||
// For Lambda compute, specifying imagePullPrincipalType is not supported | ||
const imagePullPrincipalType = this.isLambdaComputeType(this.buildImage) |
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 that imagePullPrincipalType
, secretsManagerCredentials
, and repository
will just be ignored if a Lambda-compute image is passed (I might be wrong on this).
If this is the case we could provide some feedback with Annotations.addWarningV2()
to notify the users that those properties will not be used.
I don't think we should set imagePullPrincipalType
to undefined
if this.isLambdaBuildImage(this.buildImage)
because it would either unset an already ignored value or mask a wrong property specification that should be validated.
@lpizzinidev |
} | ||
|
||
if (buildEnvironment.computeType && isLambdaComputeType(buildEnvironment.computeType)) { | ||
ret.push('Windows images do not support Lambda compute mode'); |
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.
ret.push('Windows images do not support Lambda compute mode'); | |
ret.push('Windows images do not support Lambda compute types'); |
export function isLambdaComputeType(computeType?: ComputeType): boolean { | ||
if (!computeType) return false; |
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.
export function isLambdaComputeType(computeType?: ComputeType): boolean { | |
if (!computeType) return false; | |
export function isLambdaComputeType(computeType: ComputeType): boolean { |
On second thought, let's make the property non-optional.
After all, we want to check only defined values.
// For Lambda compute, specifying imagePullPrincipalType is not supported | ||
const imagePullPrincipalType = this.isLambdaBuildImage(this.buildImage) | ||
? undefined | ||
: this.buildImage.imagePullPrincipalType === ImagePullPrincipalType.CODEBUILD | ||
? ImagePullPrincipalType.CODEBUILD | ||
: ImagePullPrincipalType.SERVICE_ROLE; |
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.
// For Lambda compute, specifying imagePullPrincipalType is not supported | |
const imagePullPrincipalType = this.isLambdaBuildImage(this.buildImage) | |
? undefined | |
: this.buildImage.imagePullPrincipalType === ImagePullPrincipalType.CODEBUILD | |
? ImagePullPrincipalType.CODEBUILD | |
: ImagePullPrincipalType.SERVICE_ROLE; | |
const imagePullPrincipalType = this.buildImage.imagePullPrincipalType === ImagePullPrincipalType.CODEBUILD | |
? ImagePullPrincipalType.CODEBUILD | |
: ImagePullPrincipalType.SERVICE_ROLE; |
Let's keep it as it was. See #27934 (comment).
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.
As mentioned in this comment, in the original implementation, imagePullCredentialsType
is always set to CODEBUILD
or SERVICE_ROLE
, causing deployment to fail and Integration Tests to fail as well.
#27934 (comment)
In renderEnvironment
method, we set registryCredential
to undefined based on the presence of buildImage.secretsManagerCredentials
when returning values.
https://github.com/aws/aws-cdk/blob/d6f6dd75383167bda980052786b85c45d9ba9e0b/packages/aws-cdk-lib/aws-codebuild/lib/project.ts#L1365C11-L1365C11
Would it be better to implement imagePullCredentialsType
in a similar way? It's essentially the same implementation, but...
return {
imagePullCredentialsType: this.isLambdaBuildImage(this.buildImage) ? undefined : imagePullPrincipalType,
};
computeType: codebuild.ComputeType.LAMBDA_1GB, | ||
}, | ||
}); | ||
}).toThrow(/Invalid CodeBuild environment: Windows images do not support Lambda compute mode/); |
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.
}).toThrow(/Invalid CodeBuild environment: Windows images do not support Lambda compute mode/); | |
}).toThrow(/Invalid CodeBuild environment: Windows images do not support Lambda compute types/); |
// eslint-disable-next-line no-duplicate-imports, import/order | ||
/* eslint-disable no-duplicate-imports, import/order */ | ||
import { LinuxArmBuildImage } from './linux-arm-build-image'; | ||
import { LinuxArmLambdaBuildImage } from './linux-arm-lambda-build-image'; | ||
import { LinuxLambdaBuildImage } from './linux-lambda-build-image'; | ||
/* eslint-enable no-duplicate-imports, import/order */ |
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.
Thanks for clarifying, let's just keep them together as you did in the original commit then. It's clearer.
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.
Thanks, just some minor adjustments and it will be good to go for me.
Note #27934 (comment).
@lpizzinidev |
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.
Thanks for the changes and the clarifications.
I missed #27934 (comment) (sorry for that).
I think we should just validate those properties in validateLambdaBuildImage
.
private validateLambdaBuildImage(buildImage: IBuildImage, props: ProjectProps): string[] { | ||
if (!this.isLambdaBuildImage(buildImage)) return []; | ||
const ret = []; | ||
if (props.timeout) { |
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.
Previously, setting imagePullPrincipalType
to CODEBUILD
seemed to cause an error (really sorry if I misunderstood). However, I just rechecked the behavior and found that deploying with imagePullPrincipalType
set to CODEBUILD
succeeds.
Therefore, I've changed the default value of imagePullPrincipalType
in the LinuxLambdaBuildImage
and LinuxArmLambdaBuildImage
classes to CODEBUILD
.
For repository
and secretsManagerCredentials
, these properties are not specified in the Lambda Build Image and are read-only, i.e., they cannot be changed after initialization in the Lambda Build Image. So, I don't think I can write unit tests to check for validation errors.
Do you mean to add own class that implements IBuildImage (like the lambda build image classes) and test it?
@@ -1361,7 +1364,7 @@ export class Project extends ProjectBase { | |||
return { | |||
type: this.buildImage.type, | |||
image: this.buildImage.imageId, | |||
imagePullCredentialsType: imagePullPrincipalType, | |||
imagePullCredentialsType: this.isLambdaBuildImage(this.buildImage) ? undefined : imagePullPrincipalType, |
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.
imagePullCredentialsType: this.isLambdaBuildImage(this.buildImage) ? undefined : imagePullPrincipalType, | |
imagePullCredentialsType: imagePullPrincipalType, |
Will remain unchanged after validating the image.
@@ -2173,3 +2228,14 @@ export enum ProjectNotificationEvents { | |||
function isBindableBuildImage(x: unknown): x is IBindableBuildImage { | |||
return typeof x === 'object' && !!x && !!(x as any).bind; | |||
} | |||
|
|||
export function isLambdaComputeType(computeType: ComputeType): boolean { | |||
if (!computeType) return false; |
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.
if (!computeType) return false; |
Always defined.
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.
sorry for missing this.
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
1 similar comment
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
…add-lambda-compute-suport
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.
Thanks for starting this @sakurai-ryo. Looks like Luca has provided some detailed reviews -- that's awesome. I have some preliminary comments, nothing too serious. Will take a deeper look in a bit.
@@ -396,6 +401,22 @@ new codebuild.Project(this, 'Project', { | |||
|
|||
Alternatively, you can reference an image available in an ECR repository using the `LinuxGpuBuildImage.fromEcrRepository(repo[, tag])` method. | |||
|
|||
### Lambda images | |||
|
|||
The `LambdaBuildImage` (or `LinuxArmLambdaBuildImage`) class contains constants for working with |
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 `LambdaBuildImage` (or `LinuxArmLambdaBuildImage`) class contains constants for working with | |
The `LinuxLambdaBuildImage` (or `LinuxArmLambdaBuildImage`) class contains constants for working with |
ret.push(`Lambda images only support ComputeTypes between '${ComputeType.LAMBDA_1GB}' and '${ComputeType.LAMBDA_10GB}' - ` + | ||
`'${buildEnvironment.computeType}' was given`); |
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.
This formatting is off. I suggest doing something like this if you want to split it into multiple lines:
ret.push([
'words',
'more words',
'final words',
].join(' '));
ret.push(`Lambda images only support ComputeTypes between ${ComputeType.LAMBDA_1GB} and ${ComputeType.LAMBDA_10GB} - ` + | ||
`${buildEnvironment.computeType} was given`); |
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 have the same issue with formatting here
*/ | ||
private validateLambdaBuildImage(buildImage: IBuildImage, props: ProjectProps): string[] { | ||
if (!this.isLambdaBuildImage(buildImage)) return []; | ||
const ret = []; |
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.
const ret = []; | |
const errors = []; |
/* eslint-disable no-duplicate-imports, import/order */ | ||
import { LinuxArmLambdaBuildImage } from './linux-arm-lambda-build-image'; | ||
import { LinuxLambdaBuildImage } from './linux-lambda-build-image'; | ||
/* eslint-enable no-duplicate-imports, import/order */ |
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.
What are the duplicate imports?
public validate(_env: BuildEnvironment): string[] { | ||
return []; | ||
public validate(env: BuildEnvironment): string[] { | ||
const ret = []; |
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.
const ret = []; | |
const errors = []; |
@@ -2068,6 +2115,15 @@ export class WindowsBuildImage implements IBuildImage { | |||
|
|||
public validate(buildEnvironment: BuildEnvironment): string[] { | |||
const ret: string[] = []; |
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.
const ret: string[] = []; | |
const errors: string[] = []; |
Pull request has been modified.
@kaizencc |
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.
A few final minor comments, and then we're good to go. Thanks @sakurai-ryo
} | ||
|
||
public readonly type = 'ARM_LAMBDA_CONTAINER'; | ||
public readonly defaultComputeType = ComputeType.LAMBDA_1GB; |
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.
what is this for / where is this used / why is this the default?
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 defaultComputeType
is required for IBuildImage
and is used if the user does not specify a computeType
.
Here is the relevant codes.
readonly defaultComputeType: ComputeType; |
computeType: env.computeType || this.buildImage.defaultComputeType, |
/* eslint-disable import/order */ | ||
import { LinuxArmLambdaBuildImage } from './linux-arm-lambda-build-image'; | ||
import { LinuxLambdaBuildImage } from './linux-lambda-build-image'; | ||
/* eslint-enable import/order */ |
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 love this actually. I think the right thing to do here is to pull ComputeType
out into its own file. Then we can import normally.
ComputeType.LAMBDA_4GB, | ||
ComputeType.LAMBDA_8GB, | ||
ComputeType.LAMBDA_10GB, | ||
].includes(computeType); |
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 going to have to continue to maintain this list every time we add a new Lambda compute type. Ideally we'd have this be an enum-like class but can't change that now. The only thing I can think of here is that instead of maintaining a list, we commit to having the BUILD_LAMBDA
prefix for all Lambda Compute types and then check for that in this function.
|
||
if (buildEnvironment.computeType && !isLambdaComputeType(buildEnvironment.computeType)) { | ||
errors.push([ | ||
'Lambda images only support ComputeTypes between', |
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.
'Lambda images only support ComputeTypes between', | |
'Lambda images only support Lambda ComputeTypes between', |
|
||
if (buildEnvironment.computeType && !isLambdaComputeType(buildEnvironment.computeType)) { | ||
errors.push([ | ||
'Lambda images only support ComputeTypes between', |
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.
'Lambda images only support ComputeTypes between', | |
'Lambda images only support Lambda ComputeTypes between', |
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.
Besides ensuring that this deploys successfully, are there any assertions we can make to confirm everything is running smoothly?
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.
It is indeed required.
Added execution of the startBuild
API on awsApiCall
.
…add-lambda-compute-suport
Pull request has been modified.
@kaizencc |
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.
Thanks for your work @sakurai-ryo!
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
CodeBuild has added support for Lambda compute. CloudFormation can be deployed as follows. ```yaml Resources: CodeBuildProject: Type: AWS::CodeBuild::Project Properties: Artifacts: Type: NO_ARTIFACTS ServiceRole: !GetAtt CodeBuildRole.Arn Source: # Environment: Type: LINUX_LAMBDA_CONTAINER ComputeType: BUILD_LAMBDA_1GB Image: aws/codebuild/amazonlinux-x86_64-lambda-standard:go1.21 CodeBuildRole: Type: AWS::IAM::Role Properties: # ``` https://aws.amazon.com/about-aws/whats-new/2023/11/aws-codebuild-lambda-compute This PR implements Lambda ComputeType by adding Classes (`LinuxArmLambdaBuildImage`, `LinuxLambdaBuildImage`) that extend the IBuildImage interface. Supported Docker Images and ComputeTypes are listed below. https://docs.aws.amazon.com/codebuild/latest/userguide/build-env-ref-available.html https://docs.aws.amazon.com/codebuild/latest/userguide/build-env-ref-compute-types.html#environment.types Also, Lambda compute has some limitations and I have added validation for them. https://docs.aws.amazon.com/codebuild/latest/userguide/lambda.html#lambda.limitations closes aws#28418 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
CodeBuild has added support for Lambda compute.
CloudFormation can be deployed as follows.
https://aws.amazon.com/about-aws/whats-new/2023/11/aws-codebuild-lambda-compute
This PR implements Lambda ComputeType by adding Classes (
LinuxArmLambdaBuildImage
,LinuxLambdaBuildImage
) that extend the IBuildImage interface.Supported Docker Images and ComputeTypes are listed below.
https://docs.aws.amazon.com/codebuild/latest/userguide/build-env-ref-available.html
https://docs.aws.amazon.com/codebuild/latest/userguide/build-env-ref-compute-types.html#environment.types
Also, Lambda compute has some limitations and I have added validation for them.
https://docs.aws.amazon.com/codebuild/latest/userguide/lambda.html#lambda.limitations
closes #28418
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license