feat(servicecatalogappregistry): initial L2 construct for Application #15140

arcrank · 2021-06-16T01:02:31Z

Service Catalog AppRegistry application construct initial base version.
Please note the ARNS for this construct have two '/' in them hence the slightly different ARN parsing.

Testing done

yarn build && yarn test
yarn integ

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

Co-authored-by: Dillon Ponzo dponzo18@gmail.com

Service Catalog AppRegistry application construct initial base version. Please note the ARNS for this construct have two '/' in them hence the slightly different ARN parsing Testing done ------------------ * `yarn build && yarn test` * `yarn integ` Co-authored-by: Dillon Ponzo <dponzo18@gmail.com> ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

gitpod-io · 2021-06-16T01:02:33Z

arcrank · 2021-06-16T01:11:33Z

Regarding ARN, the arn format is different from standard convention for this resource, however the environmentFromArn part still should work since those components of the arn, (e.g. region, account) are the same and the parsing works.

packages/@aws-cdk/aws-servicecatalogappregistry/test/application.test.ts

skinny85

Looks great! Just a few minor comments.

packages/@aws-cdk/aws-servicecatalogappregistry/README.md

packages/@aws-cdk/aws-servicecatalogappregistry/lib/application.ts

packages/@aws-cdk/aws-servicecatalogappregistry/test/integ.application.ts

Addressing comments from PR Co-authored-by: Dillon Ponzo <dponzo18@gmail.com>

Pull request has been modified.

skinny85

Looks great! A couple of super-minor issues.

packages/@aws-cdk/aws-servicecatalogappregistry/lib/application.ts

packages/@aws-cdk/aws-servicecatalogappregistry/README.md

packages/@aws-cdk/aws-servicecatalogappregistry/lib/application.ts

skinny85 · 2021-06-18T17:49:57Z

packages/@aws-cdk/aws-servicecatalogappregistry/lib/private/validation.ts

+   * Validates length is between allowed min and max lengths.
+   */
+  public static validateLength(resourceName: string, inputName: string, minLength: number, maxLength: number, inputString?: string): void {
+    if (inputString !== undefined && (inputString.length < minLength || inputString.length > maxLength)) {


One important note that I forgot to include in the Portfolio PR.

If the value provided is a dynamic CloudFormation expression, like a Parameter Ref, we should skip this validation. You can figure out whether a given value is a dynamic expression using the Token.isUnresolved() method.

This needs to be verified with a unit test.

Could you also retroactively change this in the Portfolio class?

got it. Followed convention from S3 test to make sure it doesn't throw validation error even when given an invalid value as token, which I think nicely covers the functionality.

Will make quick PR for adding this to our portfolio construct in aws-servicecatalog

btw, would there be any interest/support in making some input validation apart of core?

created PR for this in servicecatalog: #15208

packages/@aws-cdk/aws-servicecatalogappregistry/lib/private/validation.ts

Addressing minor formatting comments from PR review. Added functionality to skip validation if we are passed a token for properties, and added unit tests for those cases Testing done ------------------ * `yarn build && yarn lint && yarn test` Co-authored-by: Dillon Ponzo <dponzo18@gmail.com>

Pull request has been modified.

skinny85

Almost there!

skinny85 · 2021-06-18T23:29:28Z

packages/@aws-cdk/aws-servicecatalogappregistry/lib/private/validation.ts

+   */
+  public static validateRegex(resourceName: string, inputName: string, regex: RegExp, inputString?: string): void {
+    if (!cdk.Token.isUnresolved(inputString) && inputString !== undefined && !regex.test(inputString)) {
+      throw new Error(`Invalid ${inputName} for resource ${resourceName}, must match regex pattern ${regex}, got: '${inputString}'`);


Not truncating this one?

Didn't originally do it because it validates length first, but we truncate the string to 100 chars while the name can be longer, so probably would be better parity to do that hear as well. Will add

packages/@aws-cdk/aws-servicecatalogappregistry/lib/application.ts

packages/@aws-cdk/aws-servicecatalogappregistry/lib/private/validation.ts

skinny85 · 2021-06-18T23:39:36Z

packages/@aws-cdk/aws-servicecatalogappregistry/test/application.test.ts

+    const tokenDescription = cdk.Lazy.string({ produce: () => 'token description'.repeat(1000) });
+    expect(() => {
+      new appreg.Application(stack, 'MyApplication', {
+        applicationName: 'myApplication',
+        description: tokenDescription,
+      });
+    }).not.toThrow(/Invalid application description for resource/);


Can you use a CloudFormation Parameter here instead of a Lazy? Using Lazy like that makes the test look weird (unrealistic).

I would also just assert on the template contents:

Suggested change

const tokenDescription = cdk.Lazy.string({ produce: () => 'token description'.repeat(1000) });

expect(() => {

new appreg.Application(stack, 'MyApplication', {

applicationName: 'myApplication',

description: tokenDescription,

});

}).not.toThrow(/Invalid application description for resource/);

const param = new cdk.CfnParameter(stack, 'Param');

new appreg.Application(stack, 'MyApplication', {

applicationName: 'myApplication',

description: param.valueAsString,

});

expect(stack).toHaveResourceLike('AWS::ServiceCatalogAppRegistry::Application', {

Description: {

Ref: 'Param',

},

});

Looks like we're missing a similar test for applicationName, BTW.

updated to test template for the fields, both name and description

Updating unit tests to properly test that token inputs do end up getting created in template and not just validating it throwing a specific error Testing done ------------------ * `yarn build && yarn lint && yarn test` Co-authored-by Dillon Ponzo <dponzo18@gmail.com>

… into appregistry_l2_construct merging branch

skinny85

Looks great! 2 super minor last comments.

packages/@aws-cdk/aws-servicecatalogappregistry/README.md

packages/@aws-cdk/aws-servicecatalogappregistry/lib/application.ts

Co-authored-by: Adam Ruka <adamruka85@gmail.com>

…n.ts Co-authored-by: Adam Ruka <adamruka85@gmail.com>

arcrank · 2021-06-21T22:39:27Z

Looks great! 2 super minor last comments.

Nice turnaround on the ARN parsing. Updated the test and messaging to match our other resources for consistency.

arcrank · 2021-06-21T22:40:04Z

packages/@aws-cdk/aws-servicecatalogappregistry/lib/application.ts

+    const arn = cdk.Stack.of(scope).splitArn(applicationArn, cdk.ArnFormat.SLASH_RESOURCE_SLASH_RESOURCE_NAME);
+    const applicationId = arn.resourceName;
+
+    if (!applicationId) {


changed this to match our SC construct error check and messaging

skinny85 · 2021-06-21T23:14:08Z

packages/@aws-cdk/aws-servicecatalogappregistry/lib/application.ts

+  /**
+   * Enforces a particular physical application name.
+   */
+  readonly applicationName: string;


BTW - you called this displayName in Portfolio. Any reason you went with a different name in Application?

The actual cfn term in Portfolio is DisplayName, see here. For application it is just name hence applicationName.

You don't want to make this consistent across the ServiceCatalog constructs that exhibit this behavior (display name + auto-generated ID)?

skinny85

👍

mergify · 2021-06-22T00:11:39Z

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 · 2021-06-22T00:48:12Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
Commit ID: 48ffa72
Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2021-06-22T00:48:46Z

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#15140) Service Catalog AppRegistry application construct initial base version. Please note the ARNS for this construct have two '/' in them hence the slightly different ARN parsing. Testing done ------------------ * `yarn build && yarn test` * `yarn integ` ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* Co-authored-by: Dillon Ponzo <dponzo18@gmail.com>

arcrank changed the title ~~Initial L2 construct for appregisty application~~ feat(servicecatalogappregistry): Initial L2 construct for appregisty application Jun 16, 2021

arcrank commented Jun 16, 2021

View reviewed changes

packages/@aws-cdk/aws-servicecatalogappregistry/test/application.test.ts Show resolved Hide resolved

skinny85 self-requested a review June 16, 2021 17:38

skinny85 self-assigned this Jun 16, 2021

skinny85 previously requested changes Jun 16, 2021

View reviewed changes

Adding AppRegistry Application construct

be2e930

Addressing comments from PR Co-authored-by: Dillon Ponzo <dponzo18@gmail.com>

arcrank requested a review from skinny85 June 17, 2021 00:05

peterwoodworth added the @aws-cdk/aws-servicecatalog Related to AWS Service Catalog label Jun 17, 2021

arcrank added 2 commits June 17, 2021 12:38

Merge branch 'master' into appregistry_l2_construct

27f685c

Merge branch 'master' into appregistry_l2_construct

6911452

skinny85 previously requested changes Jun 18, 2021

View reviewed changes

Aidan Crank and others added 2 commits June 18, 2021 15:14

Merge branch 'master' into appregistry_l2_construct

40c8ba4

arcrank requested a review from skinny85 June 18, 2021 19:25

skinny85 changed the title ~~feat(servicecatalogappregistry): Initial L2 construct for appregisty application~~ feat(servicecatalogappregistry): initial L2 construct for Application Jun 18, 2021

skinny85 reviewed Jun 18, 2021

View reviewed changes

skinny85 mentioned this pull request Jun 18, 2021

fix(servicecatalog): Portfolio fails validation when passed Tokens as its properties #15208

Merged

Aidan Crank and others added 4 commits June 21, 2021 15:51

Merge branch 'appregistry_l2_construct' of github.com:arcrank/aws-cdk…

854cb01

… into appregistry_l2_construct merging branch

removing trailing whitespaces...

1ba5eed

Merge branch 'master' into appregistry_l2_construct

5b763b3

arcrank requested a review from skinny85 June 21, 2021 19:59

Merge branch 'master' into appregistry_l2_construct

427e54a

skinny85 reviewed Jun 21, 2021

View reviewed changes

packages/@aws-cdk/aws-servicecatalogappregistry/README.md Outdated Show resolved Hide resolved

packages/@aws-cdk/aws-servicecatalogappregistry/lib/application.ts Outdated Show resolved Hide resolved

Update packages/@aws-cdk/aws-servicecatalogappregistry/README.md

c652129

Co-authored-by: Adam Ruka <adamruka85@gmail.com>

arcrank and others added 3 commits June 21, 2021 18:25

Update packages/@aws-cdk/aws-servicecatalogappregistry/lib/applicatio…

be8b38b

…n.ts Co-authored-by: Adam Ruka <adamruka85@gmail.com>

Merge branch 'aws:master' into appregistry_l2_construct

46abb81

updating ARN parsing and error messaging

bbfb0f1

arcrank commented Jun 21, 2021

View reviewed changes

arcrank requested a review from skinny85 June 21, 2021 22:40

skinny85 reviewed Jun 21, 2021

View reviewed changes

skinny85 approved these changes Jun 22, 2021

View reviewed changes

Merge branch 'master' into appregistry_l2_construct

48ffa72

mergify bot merged commit 881737c into aws:master Jun 22, 2021

arcrank mentioned this pull request Jun 23, 2021

feat(servicecatalogappregistry): Initial L2 construct for Attribute Group #15282

Merged

arcrank deleted the appregistry_l2_construct branch June 30, 2021 20:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(servicecatalogappregistry): initial L2 construct for Application #15140

feat(servicecatalogappregistry): initial L2 construct for Application #15140

arcrank commented Jun 16, 2021 •

edited

Loading

gitpod-io bot commented Jun 16, 2021

arcrank commented Jun 16, 2021 •

edited

Loading

skinny85 left a comment

skinny85 left a comment

skinny85 Jun 18, 2021

arcrank Jun 18, 2021

arcrank Jun 18, 2021

arcrank Jun 18, 2021

skinny85 left a comment

skinny85 Jun 18, 2021

arcrank Jun 19, 2021

skinny85 Jun 18, 2021

arcrank Jun 21, 2021

skinny85 left a comment

arcrank commented Jun 21, 2021

arcrank Jun 21, 2021

skinny85 Jun 21, 2021

arcrank Jun 21, 2021

skinny85 Jun 21, 2021

skinny85 left a comment

mergify bot commented Jun 22, 2021

aws-cdk-automation commented Jun 22, 2021

mergify bot commented Jun 22, 2021

feat(servicecatalogappregistry): initial L2 construct for Application #15140

feat(servicecatalogappregistry): initial L2 construct for Application #15140

Conversation

arcrank commented Jun 16, 2021 • edited Loading

Testing done

gitpod-io bot commented Jun 16, 2021

arcrank commented Jun 16, 2021 • edited Loading

skinny85 left a comment

Choose a reason for hiding this comment

skinny85 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

skinny85 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

skinny85 left a comment

Choose a reason for hiding this comment

arcrank commented Jun 21, 2021

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

skinny85 left a comment

Choose a reason for hiding this comment

mergify bot commented Jun 22, 2021

aws-cdk-automation commented Jun 22, 2021

AWS CodeBuild CI Report

mergify bot commented Jun 22, 2021

arcrank commented Jun 16, 2021 •

edited

Loading

arcrank commented Jun 16, 2021 •

edited

Loading