fix(servicecatalogappregistry): modify application manager url to string #24209

zorrofox · 2023-02-17T10:03:19Z

Done

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

gitpod-io · 2023-02-17T10:03:30Z

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

✅ Updated pull request passes all PRLinter validations. Dissmissing previous PRLinter review.

aws-cdk-automation · 2023-02-17T10:50:59Z

AWS CodeBuild CI Report

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

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

comcalvi · 2023-02-27T21:41:24Z

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

-        Name: 'RAMShare5bb637032063',
+        Name: 'RAMSharee6e0e560e6f8',


Why has this been changed?

I don't know why the unique ID is changed in this unit test. And it seems all the unique IDs are changed.

This is because the hash is calculated from the application object. If the application object structure changes, the hash will change.

comcalvi · 2023-02-27T21:43:43Z

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

-    this.applicationManagerUrl = new cdk.CfnOutput(this, 'ApplicationManagerUrl', {
-      value: `https://${this.env.region}.console.aws.amazon.com/systems-manager/appmanager/application/AWS_AppRegistry_Application-${this.applicationName}`,
-      description: 'Application manager url for the application created.',
-    });
+    this.applicationManagerUrl =
+      `https://${this.env.region}.console.aws.amazon.com/systems-manager/appmanager/application/AWS_AppRegistry_Application-${this.applicationName}`;


@santanugho what do you think of this change? Why did you originally propose this to be a CfnOutput?

This defeats the purpose of providing easier access to the application URL when the application is created through TargetApplication.createApplicationStack. Since the application creation is implicit in TargetApplication constructs instead of new Application().

A new pull request #24386 was create to expose this URL as CFN output on the stack defined by TargetApplication.createApplicationStack construct, where the application is created later for stack/stage association. That got closed as duplicate.

It seems like this original design choice was not really the right choice here. Sure, it's easier for certain use cases to make this a CfnOutput, but it seems that there are use cases to do otherwise as well.

TargetApplication & ApplicationAssociator are intended to be included into default cdk template through cdk init, therefore all the cdk users can easily use the feature: all the stacks deployed by the cdk package can be contained in one application .

TargetApplication.createApplicationStack creates a new stack, defined by aws-servicecatalogappregistry, in customers account where an application is created.

For the customers who are onboard with ApplicationAssociator construct, the service team wants to provide an easier console access to the application created implicitly through TargetApplication.createApplicationStack. Therefore customers can view the application and its associated stacks without further cdk code change. Therefore the service team chose to expose the URL as CFN output in the stack, so that the console access to the application can be easily obtained. Since the CFN output stays in the stack fully managed by the aws-servicecatalogappregistry constructs, it was considered to be minimal customer impacting.

Is there any other suggestion to expose the URL while having minimal customer intervention?

I think there is a middle ground we can take:

Make Application construct having applicationManagerUrl as string type so that customers can use it to create CFN Output as needed.

In TargetApplication construct, createApplicationStack will use applicationManagerUrl property to create the CFN Output in the Stack created by the CDK construct.

Is there any other suggestion to expose the URL while having minimal customer intervention?

You can always make a boolean option: emitConsoleUrlAsOutput?: boolean.

Your proposal seems like a good middle ground.

But you will always have to contend with customer opinion, which means making it configurable. I don't mind the default being true, as long as it can be switched off for those who care.

Thanks for your comments, new PR is published: #24483

aws-cdk-automation · 2023-03-03T01:49:53Z