feat(codebuild): support images with WINDOWS_SERVER_2019_CONTAINER environment type

[ermanno]

Windows Server 2019 based images should have environment type
WINDOWS_SERVER_2019_CONTAINER. WindowsBuildImage uses the
WINDOWS_CONTAINER environment type. This patch solves this by adding a
WindowsImageType enum to the options passed the WindowsBuildImage
constructor that is used to correctly populate the type field.

fixes #9484

---

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

[ermanno]

Added a default image Windows2019BuildImage.WIN_SERVER_CORE_2019_BASE. According to this documentation and this example there is a built-in (CodeBuild-managed) image aws/codebuild/windows-base:2019-1.0 that uses WINDOWS_SERVER_2019_CONTAINER.

[skinny85]

Thanks for the contribution @ermanno ! I have an idea on how to modify the solution slightly, which (I hope!) addresses your concerns around duplication.

Let me know how you like my proposal!

Thanks,
Adam

[skinny85]

While there's nothing wrong with this solution, I have a different idea that will reduce the duplication between WindowsBuildImage and Windows2019BuildImage.

1. Let's remove Windows2019BuildImage completely for now. Everything will be part of WindowsBuildImage.
2. Let's create a public, exported enum, WindowsImageType. It will have 2 members: STANDARD, with value 'WINDOWS_CONTAINER', and SERVER_2019, with value 'WINDOWS_SERVER_2019_CONTAINER'.
3. Add an optional property imageType?: WindowsImageType to WindowsBuildImageProps.
4. In WindowsBuildImage, change type from being hard-coded in the property declaration, to being assigned in the constructor: this.type = props.imageType?.toString() ?? WindowsImageType.STANDARD.toString();.
5. Have WIN_SERVER_CORE_2019_BASE be a constant in WindowsBuildImage:

  public static readonly WIN_SERVER_CORE_2019_BASE: IBuildImage = new WindowsBuildImage({
    imageId: 'aws/codebuild/windows-base:2019-1.0',
    imagePullPrincipalType: ImagePullPrincipalType.CODEBUILD,
    imageType: WindowsImageType.SERVER_2019,
  });

6. Change WindowsBuildImage.fromDockerRegistry() to take a new interface, WindowsDockerImageOptions, instead of DockerImageOptions:

export interface WindowsDockerImageOptions extends DockerImageOptions {
  /** @default WindowsImageType.STANDARD */
  readonly imageType?: WindowsImageType;
}

7. In the implementation of WindowsBuildImage.fromDockerRegistry(), pass options.imageType when creating a new WindowsBuildImage.

[ermanno]

I will proceed to implement this change. Regarding the way we fix fromDockerRegistry with the WindowsDockerImageOptions, we also have fromEcrRepository and fromAsset... should I also add an imageType? property to a subclass of DockerImageAssetProps for the latter? For the ECR case, I don't see a way to get this information from the ecr.IRepository, so the only thing that comes to mind would be to add it to the function as an additional optional or default parameter, in which case maybe instead we should do the same thing for all three methods for consistency:

fromDockerRegistry(name: string, options: DockerImageOptions, imageType?: WindowsImageType)
fromAsset(scope: Construct, id: string, props: DockerImageAssetProps,  imageType?: WindowsImageType)
fromEcrRepository(repository: ecr.IRepository, tag: string = 'latest',  imageType?: WindowsImageType)

or

fromDockerRegistry(name: string, options: DockerImageOptions, imageType = WindowsImageType.STANDARD)
fromAsset(scope: Construct, id: string, props: DockerImageAssetProps,  imageType = WindowsImageType.STANDARD)
fromEcrRepository(repository: ecr.IRepository, tag: string = 'latest',  imageType = WindowsImageType.STANDARD)

[skinny85]

Yes, all 3 should be changed in a similar fashion, but I wouldn't do it by adding a new optional argument everywhere 🙂.

Same like I suggested creating interface WindowsDockerImageOptions extends DockerImageOptions for fromDockerRegistry(), I suggest creating interface WindowsDockerImageAssetProps extends DockerImageAssetProps for fromAsset().

The fromEcrRepository() is a harder one to crack, because of the default parameter already there... I think for that one, I'm fine with adding a third parameter with a default value, like you suggested.

[ermanno]

One difference here is that by using the Windows* classes we would change the signature of the method, and this would be a breaking/non backwards compatible change (for example, after updating CDK, old code calling WindowsBuildImage.fromDockerRegistry() with DockerImageOptions would fail to build).

By contrast, looking at the Java version of the WindowsBuildImage.fromEcrRepository() function, which has a default parameter tag, I see that if we introduce a default parameter the method is overloaded, so there are two versions of the function, one with and one without the default parameter.

Pull request has been modified.

[skinny85]

Looks great @ermanno !

Some cosmetic comments.

[skinny85]

This documentation is now out of date.

Please mention how to use the new Windows type somewhere in the ReadMe, preferably with code example(s).

[ermanno]

I have added details on how to pass the WindowsImageType optional parameter. I have also fixed a mistaken in the previous documentation: specifically, from the LinuxBuildImage and WindowsBuildImage documentation pages I see that fromCodeBuildImageId is present only in LinuxBuildImage.

Pull request has been modified.

[skinny85]

Awesome, thanks @ermanno !

[mergify]

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
• Commit ID: ecafcc1
• Result: SUCCEEDED
• Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

[mergify]

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).