chore(core): change helper functions to return response objects #904

biffgaut · 2023-02-14T20:03:39Z

Issue #, if available:
Closes #853

Description of changes:
Many helper functions currently return arrays of anonymous values. This is error prone and poorly self-documenting. This PR attempts to change our pattern to return a Response object with explicitly named attributes.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

aws-solutions-constructs-team · 2023-02-14T20:07:09Z

AWS CodeBuild CI Report

CodeBuild project: githubautobuild-for-cdk-v2
Commit ID: b5b8398
Result: FAILED
Build Logs (available for 30 days)

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

aws-solutions-constructs-team · 2023-02-14T20:58:12Z

AWS CodeBuild CI Report

CodeBuild project: githubautobuild-for-cdk-v2
Commit ID: 3fb1fb8
Result: FAILED
Build Logs (available for 30 days)

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

aws-solutions-constructs-team · 2023-02-14T21:18:47Z

AWS CodeBuild CI Report

CodeBuild project: githubautobuild-for-cdk-v2
Commit ID: c2f9d84
Result: SUCCEEDED
Build Logs (available for 30 days)

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

aws-solutions-constructs-team · 2023-02-14T21:28:22Z

AWS CodeBuild CI Report

CodeBuild project: githubautobuild-for-cdk-v2
Commit ID: b8516cb
Result: SUCCEEDED
Build Logs (available for 30 days)

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

…onstructs into Issue853

aws-solutions-constructs-team · 2023-02-15T02:48:28Z

AWS CodeBuild CI Report

CodeBuild project: githubautobuild-for-cdk-v2
Commit ID: 3a88f07
Result: FAILED
Build Logs (available for 30 days)

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

aws-solutions-constructs-team · 2023-02-15T03:00:41Z

AWS CodeBuild CI Report

CodeBuild project: githubautobuild-for-cdk-v2
Commit ID: e74093d
Result: SUCCEEDED
Build Logs (available for 30 days)

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

aws-solutions-constructs-team · 2023-02-16T22:52:30Z

AWS CodeBuild CI Report

CodeBuild project: githubautobuild-for-cdk-v2
Commit ID: e656aaa
Result: FAILED
Build Logs (available for 30 days)

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

aws-solutions-constructs-team · 2023-02-17T01:46:09Z

AWS CodeBuild CI Report

CodeBuild project: githubautobuild-for-cdk-v2
Commit ID: e8937ad
Result: SUCCEEDED
Build Logs (available for 30 days)

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

aws-solutions-constructs-team · 2023-02-17T02:39:51Z

AWS CodeBuild CI Report

CodeBuild project: githubautobuild-for-cdk-v2
Commit ID: ba253ec
Result: FAILED
Build Logs (available for 30 days)

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

aws-solutions-constructs-team · 2023-02-17T02:46:12Z

AWS CodeBuild CI Report

CodeBuild project: githubautobuild-for-cdk-v2
Commit ID: 527eb04
Result: FAILED
Build Logs (available for 30 days)

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

aws-solutions-constructs-team · 2023-02-17T02:54:51Z

AWS CodeBuild CI Report

CodeBuild project: githubautobuild-for-cdk-v2
Commit ID: 3b03dbe
Result: FAILED
Build Logs (available for 30 days)

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

aws-solutions-constructs-team · 2023-02-17T03:11:22Z

AWS CodeBuild CI Report

CodeBuild project: githubautobuild-for-cdk-v2
Commit ID: 7e9d682
Result: FAILED
Build Logs (available for 30 days)

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

aws-solutions-constructs-team · 2023-02-17T03:22:41Z

AWS CodeBuild CI Report

CodeBuild project: githubautobuild-for-cdk-v2
Commit ID: 085e098
Result: SUCCEEDED
Build Logs (available for 30 days)

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

aws-solutions-constructs-team · 2023-02-17T03:32:44Z

AWS CodeBuild CI Report

CodeBuild project: githubautobuild-for-cdk-v2
Commit ID: ea61071
Result: SUCCEEDED
Build Logs (available for 30 days)

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

aws-solutions-constructs-team · 2023-02-17T16:38:25Z

AWS CodeBuild CI Report

CodeBuild project: githubautobuild-for-cdk-v2
Commit ID: 0851826
Result: SUCCEEDED
Build Logs (available for 30 days)

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

aws-solutions-constructs-team · 2023-02-17T19:15:56Z

AWS CodeBuild CI Report

CodeBuild project: githubautobuild-for-cdk-v2
Commit ID: 17b3476
Result: SUCCEEDED
Build Logs (available for 30 days)

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

aws-solutions-constructs-team · 2023-02-17T22:16:58Z

AWS CodeBuild CI Report

CodeBuild project: githubautobuild-for-cdk-v2
Commit ID: 1826fe3
Result: FAILED
Build Logs (available for 30 days)

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

aws-solutions-constructs-team · 2023-02-17T22:24:47Z

AWS CodeBuild CI Report

CodeBuild project: githubautobuild-for-cdk-v2
Commit ID: c9b1e7e
Result: FAILED
Build Logs (available for 30 days)

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

aws-solutions-constructs-team · 2023-02-17T22:33:51Z

AWS CodeBuild CI Report

CodeBuild project: githubautobuild-for-cdk-v2
Commit ID: d35a88b
Result: FAILED
Build Logs (available for 30 days)

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

aws-solutions-constructs-team · 2023-02-17T22:48:23Z

AWS CodeBuild CI Report

CodeBuild project: githubautobuild-for-cdk-v2
Commit ID: ad4e3c9
Result: SUCCEEDED
Build Logs (available for 30 days)

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

aws-solutions-constructs-team · 2023-02-18T02:01:28Z

AWS CodeBuild CI Report

CodeBuild project: githubautobuild-for-cdk-v2
Commit ID: dbaa181
Result: SUCCEEDED
Build Logs (available for 30 days)

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

aws-solutions-constructs-team · 2023-02-18T19:15:41Z

AWS CodeBuild CI Report

CodeBuild project: githubautobuild-for-cdk-v2
Commit ID: c5c048b
Result: SUCCEEDED
Build Logs (available for 30 days)

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

aws-solutions-constructs-team · 2023-02-19T18:22:44Z

AWS CodeBuild CI Report

CodeBuild project: githubautobuild-for-cdk-v2
Commit ID: a39986f
Result: SUCCEEDED
Build Logs (available for 30 days)

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

georgebearden

Overall, it looks good to me. Just a handful of minor comments.

georgebearden · 2023-02-20T14:32:06Z

source/patterns/@aws-solutions-constructs/aws-apigateway-dynamodb/lib/index.ts

      existingTableObj: props.existingTableObj,
      dynamoTableProps: props.dynamoTableProps
-    })[1] as dynamodb.Table;
+    });
+    this.dynamoTable = buildDynamoDBTableResponse.tableObject!;


When I first read this, I asked myself "If we know the tableObject is never null, why not change the helper to return that". After going through the whole review it makes sense why this was done with the ! operator where possible. Nice check!

georgebearden · 2023-02-20T15:06:52Z

source/patterns/@aws-solutions-constructs/aws-s3-sns/test/integ.existingSnsTopic.ts

@@ -21,12 +21,12 @@ const app = new App();
 const stack = new Stack(app, generateIntegStackName(__filename));

 const existingTopicEncryptionKey = defaults.buildEncryptionKey(stack, {});
-const [ existingTopicObj ] = defaults.buildTopic(stack, {
+const buildTopicRepsponse = defaults.buildTopic(stack, {


Nit - typo buildTopicResponse

georgebearden · 2023-02-20T15:07:22Z

source/patterns/@aws-solutions-constructs/aws-s3-sns/test/integ.existingSnsTopic.ts

  encryptionKey: existingTopicEncryptionKey
 });

 new S3ToSns(stack, 'test-s3-sns', {
-  existingTopicObj,
+  existingTopicObj: buildTopicRepsponse.topic,


georgebearden · 2023-02-20T15:10:17Z

source/patterns/@aws-solutions-constructs/core/lib/apigateway-helper.ts

@@ -15,7 +15,7 @@
 import * as logs from 'aws-cdk-lib/aws-logs';
 import * as lambda from 'aws-cdk-lib/aws-lambda';
 import * as cdk from 'aws-cdk-lib';
-import * as api from 'aws-cdk-lib/aws-apigateway';
+import * as apig from 'aws-cdk-lib/aws-apigateway';


Minor and don't necessarily need to change, but api and apig are still really close when looking at the code. I would prefer this import to be apigateway (which also matches the convention of the other imports in this file.

georgebearden · 2023-02-20T15:11:05Z

source/patterns/@aws-solutions-constructs/core/lib/cloudfront-distribution-helper.ts

 export function CloudFrontDistributionForApiGateway(scope: Construct,
  apiEndPoint: api.RestApi,
  cloudFrontDistributionProps?: cloudfront.DistributionProps | any,
  httpSecurityHeaders: boolean = true,
  cloudFrontLoggingBucketProps?: s3.BucketProps,
  responseHeadersPolicyProps?: cloudfront.ResponseHeadersPolicyProps
-): [cloudfront.Distribution, cloudfront.Function?, s3.Bucket?] {
+): CloudFrontDistributionForApiGatewayResponse {


This PR as a whole is great, and I really like this change!

georgebearden · 2023-02-20T15:11:55Z

source/patterns/@aws-solutions-constructs/core/lib/cloudfront-distribution-helper.ts

@@ -190,7 +208,7 @@ export function CloudFrontDistributionForMediaStore(scope: Construct,
  const cfDistribution = new cloudfront.Distribution(scope, 'CloudFrontDistribution', cfprops);
  updateSecurityPolicy(cfDistribution);

-  return [cfDistribution, loggingBucket, originRequestPolicy, cloudfrontFunction];
+  return { distribution: cfDistribution, loggingBucket, requestPolicy: originRequestPolicy, cloudfrontFunction};


Nit - space before the closing }

georgebearden · 2023-02-20T15:14:01Z

source/patterns/@aws-solutions-constructs/core/lib/glue-job-helper.ts

+      const deployGlueJobResponse =
+        deployGlueJob(scope, props.glueJobProps, props.database!, props.table!, props.outputDataStore!, props.etlCodeAsset);
+      return {
+        job: deployGlueJobResponse.job,
+        role: deployGlueJobResponse.role,
+        bucket: deployGlueJobResponse.bucket,
+        loggingBucket: deployGlueJobResponse.loggingBucket };


Can this be simplified to just:

return deployGlueJob(scope, props.glueJobProps, props.database!, props.table!, props.outputDataStore!, props.etlCodeAsset);

The fields look the same at a glance, but maybe they aren't the same types or something, let me know.

Yes, that would work and is what happened in the past. But they are different types that coincidentally have the same attributes. To return one type in place of the other because we can currently get away with it would be brittle and couple the two routines.

georgebearden · 2023-02-20T15:14:45Z

source/patterns/@aws-solutions-constructs/core/lib/glue-job-helper.ts

 export function deployGlueJob(scope: Construct, glueJobProps: glue.CfnJobProps, database: glue.CfnDatabase, table: glue.CfnTable,
-  outputDataStore: SinkDataStoreProps, etlCodeAsset?: s3assets.Asset): [glue.CfnJob, IRole, [Bucket, (Bucket | undefined)?]] {
+  outputDataStore: SinkDataStoreProps, etlCodeAsset?: s3assets.Asset): DeployGlueJobResponse {
+  // TODO - replicate [glue.CfnJob, IRole, [Bucket, (Bucket | undefined)?]]


I'm not sure what this TODO means? Let's add some more details so it makes sense to the next person, or clean it up!

It just got left behind, the work was done. I will remove.

georgebearden · 2023-02-20T15:17:38Z

source/patterns/@aws-solutions-constructs/core/lib/glue-job-helper.ts

@@ -143,57 +165,57 @@ export function deployGlueJob(scope: Construct, glueJobProps: glue.CfnJobProps,
    ]
  });

-  let _jobRole: IRole;
+  let jobRole: IRole;


If you prefer to define jobRole as const

const jobRole = glueJobProps.role ? Role.fromRoleArn(scope, 'JobRole', glueJobProps.role) : defaults.createGlueJobRole(scope);

OK, but formatting differently for clarity

aws-solutions-constructs-team · 2023-02-20T16:24:48Z

AWS CodeBuild CI Report

CodeBuild project: githubautobuild-for-cdk-v2
Commit ID: a85f6ae
Result: SUCCEEDED
Build Logs (available for 30 days)

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

georgebearden

looks good to me.

Changed API functions to return response objects

b5b8398

biffgaut requested review from hayesry, mobri2a, emcfins, mickychetta and georgebearden as code owners February 14, 2023 20:03

Fix remaining references to API helper functions

3fb1fb8

One more reference to API Helper function

c2f9d84

Merge branch 'main' into Issue853

b8516cb

biffgaut added 3 commits February 14, 2023 20:41

CloudFront and lambda-sagemakerendpoint

e667e89

updated dynamodb

7a8f9d8

Merge branch 'Issue853' of https://github.com/awslabs/aws-solutions-c…

3a88f07

…onstructs into Issue853

Format errors

e74093d

biffgaut added 2 commits February 15, 2023 10:48

Adjust Elasticsearch

20ca283

Includes S3, apigateway

e656aaa

Missed Apigateway reference

e8937ad

Upated fargate-helper

ba253ec

One missed fargate reference

527eb04

One more fargate

3b03dbe

More fargate references

7e9d682

missing whitespace

085e098

Merge branch 'main' into Issue853

ea61071

all sns changes

0851826

step functions

17b3476

sqs referencex

1826fe3

Another sqs reference

c9b1e7e

lambda-sqs-lambda

d35a88b

one more

ad4e3c9

nettlesome flummery

dbaa181

Sagemaker Return values

c5c048b

Clean up, ensure consistency

a39986f

georgebearden reviewed Feb 20, 2023

View reviewed changes

Address Review Issues

a85f6ae

georgebearden approved these changes Feb 20, 2023

View reviewed changes

biffgaut merged commit de47a16 into main Feb 20, 2023

biffgaut deleted the Issue853 branch February 20, 2023 16:42

chore(core): change helper functions to return response objects #904

chore(core): change helper functions to return response objects #904

Conversation

biffgaut commented Feb 14, 2023

aws-solutions-constructs-team commented Feb 14, 2023

AWS CodeBuild CI Report

aws-solutions-constructs-team commented Feb 14, 2023

AWS CodeBuild CI Report

aws-solutions-constructs-team commented Feb 14, 2023

AWS CodeBuild CI Report

aws-solutions-constructs-team commented Feb 14, 2023

AWS CodeBuild CI Report

aws-solutions-constructs-team commented Feb 15, 2023

AWS CodeBuild CI Report

aws-solutions-constructs-team commented Feb 15, 2023

AWS CodeBuild CI Report

aws-solutions-constructs-team commented Feb 16, 2023

AWS CodeBuild CI Report

aws-solutions-constructs-team commented Feb 17, 2023

AWS CodeBuild CI Report

aws-solutions-constructs-team commented Feb 17, 2023

AWS CodeBuild CI Report

aws-solutions-constructs-team commented Feb 17, 2023

AWS CodeBuild CI Report

aws-solutions-constructs-team commented Feb 17, 2023

AWS CodeBuild CI Report

aws-solutions-constructs-team commented Feb 17, 2023

AWS CodeBuild CI Report

aws-solutions-constructs-team commented Feb 17, 2023

AWS CodeBuild CI Report

aws-solutions-constructs-team commented Feb 17, 2023

AWS CodeBuild CI Report

aws-solutions-constructs-team commented Feb 17, 2023

AWS CodeBuild CI Report

aws-solutions-constructs-team commented Feb 17, 2023

AWS CodeBuild CI Report

aws-solutions-constructs-team commented Feb 17, 2023

AWS CodeBuild CI Report

aws-solutions-constructs-team commented Feb 17, 2023

AWS CodeBuild CI Report

aws-solutions-constructs-team commented Feb 17, 2023

AWS CodeBuild CI Report

aws-solutions-constructs-team commented Feb 17, 2023

AWS CodeBuild CI Report

aws-solutions-constructs-team commented Feb 18, 2023

AWS CodeBuild CI Report

aws-solutions-constructs-team commented Feb 18, 2023

AWS CodeBuild CI Report

aws-solutions-constructs-team commented Feb 19, 2023

AWS CodeBuild CI Report

georgebearden left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aws-solutions-constructs-team commented Feb 20, 2023

AWS CodeBuild CI Report

georgebearden left a comment

Choose a reason for hiding this comment