fix(codedeploy): DeploymentGroup's physical name not propagated to Application #13582

BLasan · 2021-03-14T07:33:46Z

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

gitpod-io · 2021-03-14T07:33:48Z

BLasan · 2021-03-14T16:49:19Z

@skinny85 Please review

rix0rrr · 2021-03-15T10:09:51Z

packages/@aws-cdk/aws-codedeploy/lib/server/deployment-group.ts

@@ -269,7 +269,7 @@ export class ServerDeploymentGroup extends ServerDeploymentGroupBase {
      physicalName: props.deploymentGroupName,
    });

-    this.application = props.application || new ServerApplication(this, 'Application');
+    this.application = props.application || new ServerApplication(this, this.physicalName);


This is not the right fix for this issue. The fix is that if the parent DeploymentGroup has a PhysicalName.GENERATE_IF_NEEDED, so should the ServerApplication.

The poster suggested as much under Expected Result.

Also the title of this PR doesn't cover the change it's making. The title as written makes it sound as if no Application was created before, whereas now there is. This is not true though, an Application was already being created before.

We generally also write the PR title in terms of things the user would care about, and specifically for fixes it describes the bug that is now fixed (users will see these titles in the CHANGELOG, so think about what would make sense to them when viewed there).

@rix0rrr Thanks for the review. I made the changes. Please have a look

Pull request has been modified.

rix0rrr · 2021-03-16T08:22:06Z

packages/@aws-cdk/aws-codedeploy/lib/server/deployment-group.ts

@@ -4,6 +4,7 @@ import * as ec2 from '@aws-cdk/aws-ec2';
 import * as iam from '@aws-cdk/aws-iam';
 import * as s3 from '@aws-cdk/aws-s3';
 import * as cdk from '@aws-cdk/core';
+import { isGeneratedWhenNeededMarker } from '@aws-cdk/core/lib/private/physical-name-generator';


You can't import this in this way. This looks like it's a private function. Either import from @aws-cdk/core directory, or if it's not exported there, you can't import this at all.

I'm guessing name == PhysicalName.GENERATE_IF_NEEDED should just work, did you try that?

I initially tried cdk.PhysicalName.GENERATE_IF_NEEDED . But after going through the logic resides inside the resources.ts file, I came up with this logic :) I hope name == PhysicalName.GENERATE_IF_NEEDED would work fine. will changes that

rix0rrr · 2021-03-16T08:23:35Z

packages/@aws-cdk/aws-codedeploy/lib/server/deployment-group.ts

@@ -269,7 +270,11 @@ export class ServerDeploymentGroup extends ServerDeploymentGroupBase {
      physicalName: props.deploymentGroupName,
    });

-    this.application = props.application || new ServerApplication(this, 'Application');
+    if (props.deploymentGroupName && isGeneratedWhenNeededMarker(props.deploymentGroupName)) {
+      this.application = new ServerApplication(this, this.physicalName);


You are changing the construct ID, not the resource name. The construct ID needs to stay consistent (and cannot be a token). The resource name is a property and CAN be a token.

The pattern always looks like this:

new Resource(this, 'ResourceId', { // <--- id resourceName: 'resourceName' // <--- name });

rix0rrr · 2021-03-16T08:25:47Z

packages/@aws-cdk/aws-codedeploy/test/server/test.deployment-group.ts

+        deploymentGroupName: physical_name,
+      });
+
+      test.notEqual(deploymentGroup, undefined);


The test you want to write here is that there is an AWS::CodeDeploy::Application with a specific ApplicationName property.

Look into using haveResource assertions.

Pull request has been modified.

BLasan · 2021-03-16T10:08:34Z

@rix0rrr Please review

rix0rrr · 2021-03-16T12:30:21Z

packages/@aws-cdk/aws-codedeploy/test/server/test.deployment-group.ts

+        deploymentGroupName: cdk.PhysicalName.GENERATE_IF_NEEDED,
+      });
+
+      expect(stack).to(haveResource('AWS::CodeDeploy::DeploymentGroup', {


You need to be asserting something about the AWS::CodeDeploy::Application, not the DeploymentGroup.

Yuh, but there won't be a field called ApplicationName in the output. That fails the build.

Then the issue is not fixed. That is what the desired output needs to be.

If it's not that, then we need to change code until it is.

- Field ApplicationName missing in: { "Type": "AWS::CodeDeploy::Application", "Properties": { "ComputePlatform": "Server" } }

output for the test case with the resource type AWS::CodeDeploy:;DeploymentGroup. Same output will be received when the resource type of can be created by explicitly passing an Application test changed

Pull request has been modified.

rix0rrr · 2021-03-17T09:56:31Z

packages/@aws-cdk/aws-codedeploy/test/server/test.deployment-group.ts

+      });
+
+      expect(stack).to(haveResource('AWS::CodeDeploy::Application', {
+        'ComputePlatform': 'Server',


We do need an ApplicationName provided here.

If it doesn't appear, then the fix is not sufficient. It may be the that the ServerApplication does not respect PhysicalNames, for example.

Try and step through the code to see where the discrepancy between what we expect and what actually happens comes from.

Can we give a custom name and check whether we'll be receiving the out as expected instead of a PhysicaName?

…ate_if_needed

…plication with physical name if needed

…region/account

Pull request has been modified.

mergify · 2021-03-22T09:59:53Z

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-03-22T10:31:59Z

AWS CodeBuild CI Report

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

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

mergify · 2021-03-22T10:32:24Z

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

…pplication (#13582) fixes #13337 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

…pplication (aws#13582) fixes aws#13337 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

github-actions bot added the @aws-cdk/aws-codedeploy Related to AWS CodeDeploy label Mar 14, 2021

github-actions bot assigned skinny85 Mar 14, 2021

BLasan force-pushed the issue-13337 branch 2 times, most recently from 198c2e5 to fdb8af8 Compare March 14, 2021 08:07

rix0rrr previously requested changes Mar 15, 2021

View reviewed changes

skinny85 assigned rix0rrr and unassigned skinny85 Mar 15, 2021

BLasan force-pushed the issue-13337 branch from fdb8af8 to c38bb86 Compare March 16, 2021 07:46

BLasan force-pushed the issue-13337 branch from c38bb86 to 9d39e8f Compare March 16, 2021 07:49

rix0rrr previously requested changes Mar 16, 2021

View reviewed changes

BLasan force-pushed the issue-13337 branch from 9d39e8f to 5cee8d4 Compare March 16, 2021 09:22

BLasan changed the title ~~fix(aws-codedeploy): implicitly create application in ServerDeploymentGroup~~ fix(aws-codedeploy): deploymentgroup creation with physicalname_generate_if_needed Mar 16, 2021

github-actions bot assigned skinny85 Mar 16, 2021

BLasan force-pushed the issue-13337 branch from e35e3b5 to b360450 Compare March 16, 2021 11:16

rix0rrr previously requested changes Mar 16, 2021

View reviewed changes

BLasan force-pushed the issue-13337 branch from b360450 to dcc7061 Compare March 16, 2021 20:09

BLasan force-pushed the issue-13337 branch 2 times, most recently from 6f5622a to 36020c4 Compare March 16, 2021 20:24

rix0rrr previously requested changes Mar 17, 2021

View reviewed changes

BLasan added 3 commits March 19, 2021 11:41

fix(aws-codedeploy): deploymentgroup creation with physicalname_gener…

ea4c0ee

…ate_if_needed

test(aws-codedeploy): change expected resource type in creating an ap…

bad695d

…plication with physical name if needed

test(aws-codedeploy-server): add referencing from a stack in another …

51cc96d

…region/account

BLasan force-pushed the issue-13337 branch from 36020c4 to 51cc96d Compare March 19, 2021 06:11

skinny85 removed their assignment Mar 19, 2021

rix0rrr changed the title ~~fix(aws-codedeploy): deploymentgroup creation with physicalname_generate_if_needed~~ fix(codedeploy): DeploymentGroup's physical name not propagated to Application Mar 22, 2021

rix0rrr approved these changes Mar 22, 2021

View reviewed changes

github-actions bot assigned skinny85 Mar 22, 2021

Merge branch 'master' into issue-13337

8f6ad60

mergify bot merged commit 9d228e4 into aws:master Mar 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(codedeploy): DeploymentGroup's physical name not propagated to Application #13582

fix(codedeploy): DeploymentGroup's physical name not propagated to Application #13582

BLasan commented Mar 14, 2021

gitpod-io bot commented Mar 14, 2021 •

edited

Loading

BLasan commented Mar 14, 2021

rix0rrr Mar 15, 2021

BLasan Mar 16, 2021

rix0rrr Mar 16, 2021

BLasan Mar 16, 2021

rix0rrr Mar 16, 2021

rix0rrr Mar 16, 2021

BLasan commented Mar 16, 2021

rix0rrr Mar 16, 2021

BLasan Mar 16, 2021

rix0rrr Mar 16, 2021 •

edited

Loading

BLasan Mar 16, 2021

rix0rrr Mar 17, 2021

BLasan Mar 17, 2021

mergify bot commented Mar 22, 2021

aws-cdk-automation commented Mar 22, 2021

mergify bot commented Mar 22, 2021

fix(codedeploy): DeploymentGroup's physical name not propagated to Application #13582

fix(codedeploy): DeploymentGroup's physical name not propagated to Application #13582

Conversation

BLasan commented Mar 14, 2021

gitpod-io bot commented Mar 14, 2021 • edited Loading

BLasan commented Mar 14, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BLasan commented Mar 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rix0rrr Mar 16, 2021 • edited Loading

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

mergify bot commented Mar 22, 2021

aws-cdk-automation commented Mar 22, 2021

AWS CodeBuild CI Report

mergify bot commented Mar 22, 2021

gitpod-io bot commented Mar 14, 2021 •

edited

Loading

rix0rrr Mar 16, 2021 •

edited

Loading