feat(gamelift): support Build serverSdkVersion, updated OperatingSystem values #27857

lpizzinidev · 2023-11-06T16:29:13Z

Some adjustments to Build properties:

Adds support for serverSdkVersion attribute
Updates list of available operatingSystem and deprecates Windows Server 2012

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

…em values

scanlonp

Looks great! ~~A minor nit on the error message, and then it should be good to go.~~ Edited the error message.

One concern. Should serverSdkVersion be a public attribute on the Build and Import classes? It looks like the rest of the parameters are.

packages/@aws-cdk/aws-gamelift-alpha/lib/build.ts

packages/@aws-cdk/aws-gamelift-alpha/test/build.test.ts

Pull request has been modified.

go-to-k

Looks good to me. I had to comment on a couple of points.

go-to-k · 2023-11-07T06:18:19Z

packages/@aws-cdk/aws-gamelift-alpha/lib/build.ts

@@ -276,4 +313,13 @@ export class Build extends BuildBase {
    });
  }

+  private validateServerSdkVersion(serverSdkVersion?: string) {
+    if (!serverSdkVersion) return;


It will be passed if a string is empty, so I would prefer an undefined check. And I think it should take the case that the param is Token by CfnParameter, etc., what do you think?

Suggested change

if (!serverSdkVersion) return;

if (serverSdkVersion === undefined || Token.isUnresolved(serverSdkVersion)) return;

go-to-k · 2023-11-07T06:24:14Z

packages/@aws-cdk/aws-gamelift-alpha/lib/build.ts

+      throw new Error(`serverSdkVersion must be in the 0.0.0 format, got ${serverSdkVersion}.`);
+    }
+    if (serverSdkVersion.length > 128) {
+      throw new Error(`serverSdkVersion length must be smaller than or equal to 128, got ${serverSdkVersion}.`);


I think it would be better to output the length of a character in the error message. I have seen other cases like this.

https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/sagemaker/create-training-job.ts#L341
https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk-lib/aws-events-targets/lib/batch.ts#L118
https://github.com/aws/aws-cdk/blob/main/packages/%40aws-cdk/aws-glue-alpha/lib/database.ts#L136

Suggested change

throw new Error(`serverSdkVersion length must be smaller than or equal to 128, got ${serverSdkVersion}.`);

throw new Error(`serverSdkVersion length must be smaller than or equal to 128, got ${serverSdkVersion.length}.`);

go-to-k · 2023-11-07T06:37:24Z

Oh, sorry, @scanlonp already reviewed.

lpizzinidev · 2023-11-07T07:32:58Z

@go-to-k
Nice catches! I missed both of them 😄

@scanlonp
Some of the props are exposed, but others are not.
Unless there is a specific need to provide a reference to serverSdkVersion outside of the class I would keep it private.

go-to-k

@lpizzinidev Thanks! I approved.

mergify · 2023-11-08T23:31:14Z

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).

mergify · 2023-11-08T23:34:48Z

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-cdk-automation · 2023-11-09T00:14:07Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 15de967
Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2023-11-09T00:14:27Z

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).

…em values (#27857) Some adjustments to `Build` properties: * Adds support for [`serverSdkVersion`](https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-gamelift-build.html#cfn-gamelift-build-serversdkversion) attribute * Updates list of available [`operatingSystem`](https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-gamelift-build.html#cfn-gamelift-build-operatingsystem) and deprecates Windows Server 2012 Closes #27655. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

feat(gamelift): support Build serverSdkVersion, updated OperatingSyst…

3aa3ff2

…em values

github-actions bot added distinguished-contributor [Pilot] contributed 50+ PRs to the CDK effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p2 labels Nov 6, 2023

aws-cdk-automation requested a review from a team November 6, 2023 16:30

scanlonp previously requested changes Nov 6, 2023

View reviewed changes

packages/@aws-cdk/aws-gamelift-alpha/lib/build.ts Outdated Show resolved Hide resolved

packages/@aws-cdk/aws-gamelift-alpha/test/build.test.ts Outdated Show resolved Hide resolved

scanlonp added 2 commits November 6, 2023 08:47

Update packages/@aws-cdk/aws-gamelift-alpha/lib/build.ts

affb55e

Update packages/@aws-cdk/aws-gamelift-alpha/test/build.test.ts

9ff0d08

aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Nov 6, 2023

go-to-k suggested changes Nov 7, 2023

View reviewed changes

fixed serverSdkVersion validation

640044b

go-to-k approved these changes Nov 7, 2023

View reviewed changes

aws-cdk-automation added pr/needs-maintainer-review This PR needs a review from a Core Team Member and removed pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. labels Nov 7, 2023

vinayak-kukreja assigned scanlonp Nov 8, 2023

scanlonp approved these changes Nov 8, 2023

View reviewed changes

aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Nov 8, 2023

Merge branch 'main' into awsgh-27655

15de967

mergify bot merged commit f1bb801 into aws:main Nov 9, 2023
9 checks passed

github-actions bot mentioned this pull request Dec 1, 2023

Monthly PR metrics report - November 2023 #28212

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(gamelift): support Build serverSdkVersion, updated OperatingSystem values #27857

feat(gamelift): support Build serverSdkVersion, updated OperatingSystem values #27857

lpizzinidev commented Nov 6, 2023

scanlonp left a comment •

edited

Loading

go-to-k left a comment

go-to-k Nov 7, 2023

go-to-k Nov 7, 2023

go-to-k commented Nov 7, 2023 •

edited

Loading

lpizzinidev commented Nov 7, 2023

go-to-k left a comment

mergify bot commented Nov 8, 2023

mergify bot commented Nov 8, 2023

aws-cdk-automation commented Nov 9, 2023

mergify bot commented Nov 9, 2023

	if (!serverSdkVersion) return;
	if (serverSdkVersion === undefined \|\| Token.isUnresolved(serverSdkVersion)) return;

	throw new Error(`serverSdkVersion length must be smaller than or equal to 128, got ${serverSdkVersion}.`);
	throw new Error(`serverSdkVersion length must be smaller than or equal to 128, got ${serverSdkVersion.length}.`);

feat(gamelift): support Build serverSdkVersion, updated OperatingSystem values #27857

feat(gamelift): support Build serverSdkVersion, updated OperatingSystem values #27857

Conversation

lpizzinidev commented Nov 6, 2023

scanlonp left a comment • edited Loading

Choose a reason for hiding this comment

go-to-k left a comment

Choose a reason for hiding this comment

go-to-k Nov 7, 2023

Choose a reason for hiding this comment

go-to-k Nov 7, 2023

Choose a reason for hiding this comment

go-to-k commented Nov 7, 2023 • edited Loading

lpizzinidev commented Nov 7, 2023

go-to-k left a comment

Choose a reason for hiding this comment

mergify bot commented Nov 8, 2023

mergify bot commented Nov 8, 2023

aws-cdk-automation commented Nov 9, 2023

AWS CodeBuild CI Report

mergify bot commented Nov 9, 2023

scanlonp left a comment •

edited

Loading

go-to-k commented Nov 7, 2023 •

edited

Loading