Skip to content

Conversation

@leonmk-aws
Copy link
Contributor

@leonmk-aws leonmk-aws commented Oct 10, 2025

Issue # (if applicable)

Closes #.

Reason for this change

Allow passing L2s and L1s to L1s

Description of changes

Added a new relationship decider to parse the relationship information from the database and allow the other deciders to modify the properties / constructors accordingly. The relationship deciders assumes that:

  • All modules will be built
  • The properties in the relationship will exist (and have the same naming)

This generates code that looks like this for non nested properties:

In the properties:
readonly role: IamIRoleRef | string;

This is then used in the constructor:
this.role = (props.role as IamIRoleRef)?.roleRef?.roleArn ?? props.role;
If there were multiple possible IxxRef:
(props.targetArn as SqsIQueueRef)?.queueRef?.queueArn ?? (props.targetArn as SnsITopicRef)?.topicRef?.topicArn ?? props.targetArn

For nested properties

The props behave the same way, "flatten" functions are generated to recursively perform the same role that was done in the constructor for non nested properties:

function flattenCfnIdentityPoolRoleAttachmentRoleMappingProperty(props: CfnIdentityPoolRoleAttachment.RoleMappingProperty): CfnIdentityPoolRoleAttachment.RoleMappingProperty {
  return {
    "ambiguousRoleResolution": props.ambiguousRoleResolution,
    "identityProvider": props.identityProvider,
    "rulesConfiguration": (cdk.isResolvableObject(props.rulesConfiguration) ? props.rulesConfiguration : (props.rulesConfiguration ? flattenCfnIdentityPoolRoleAttachmentRulesConfigurationTypeProperty(props.rulesConfiguration) : undefined)),
    "type": props.type
  };
}

For arrays

 // @ts-ignore TS6133
function flattenCfnCodeSigningConfigAllowedPublishersProperty(props: CfnCodeSigningConfig.AllowedPublishersProperty | cdk.IResolvable): CfnCodeSigningConfig.AllowedPublishersProperty | cdk.IResolvable {
  if (cdk.isResolvableObject(props)) return props;
  return {
    "signingProfileVersionArns": props.signingProfileVersionArns?.map((item: any) => (item as SignerISigningProfileRef)?.signingProfileRef?.signingProfileArn ?? item)
  };
}

For arrays of nested props:

    this.fileSystemConfigs = (cdk.isResolvableObject(props.fileSystemConfigs) ? props.fileSystemConfigs : (props.fileSystemConfigs ? props.fileSystemConfigs.map(flattenCfnFunctionFileSystemConfigProperty) : undefined));

Database assumptions

Properties (the properties in sense of props of a resource but also prop of a type) have a a relationshipRefs array containing the relationship information:
relationshipRefs: [{ typeName: "AWS::IAM::Role", propertyPath: "Arn" }, ...]

Description of how you validated changes

  • Checked the diffs between the previously generated code and the new one.
  • Added snapshot tests
  • Added unit tests for lambda
  • Deployed a stack manually consisting of mixes of L1 and L2 resources using the new capabilities this PR adds

Checklist


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

@github-actions github-actions bot added the p2 label Oct 10, 2025
@aws-cdk-automation aws-cdk-automation requested a review from a team October 10, 2025 08:33
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Oct 10, 2025
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(This review is outdated)

@rix0rrr rix0rrr changed the title feat: passing L2s and L1s to L1s feat: L1s can now accept other resources as parameters where relationships are expected Oct 10, 2025
@rix0rrr rix0rrr changed the title feat: L1s can now accept other resources as parameters where relationships are expected feat(core): L1s can now accept other resources as parameters where relationships are expected Oct 10, 2025
@rix0rrr rix0rrr added pr-linter/exempt-readme The PR linter will not require README changes pr-linter/exempt-integ-test The PR linter will not require integ test changes labels Oct 10, 2025
@rix0rrr rix0rrr changed the title feat(core): L1s can now accept other resources as parameters where relationships are expected feat(core): L1s can now accept constructs as parameters for known resource relationships Oct 10, 2025
@leonmk-aws leonmk-aws added the pr/do-not-merge This PR should not be merged at this time. label Oct 10, 2025
@leonmk-aws leonmk-aws force-pushed the leonmk-harmony branch 2 times, most recently from 624f91c to 7ec3cc9 Compare October 13, 2025 07:33
@leonmk-aws leonmk-aws marked this pull request as ready for review October 13, 2025 08:36
@rix0rrr rix0rrr changed the title feat(core): L1s can now accept constructs as parameters for known resource relationships feat(core): Cfn constructs (L1s) can now accept constructs as parameters for known resource relationships Oct 14, 2025
@rix0rrr rix0rrr changed the title feat(core): Cfn constructs (L1s) can now accept constructs as parameters for known resource relationships feat(core): cfn constructs (L1s) can now accept constructs as parameters for known resource relationships Oct 14, 2025
@aws-cdk-automation aws-cdk-automation dismissed their stale review October 14, 2025 08:49

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

Copy link
Contributor

@rix0rrr rix0rrr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a unit tests somewhere that looks like this:

test('it works', () => {
  const stack = new Stack();

  const cfnRole = new CfnRole(stack, ...);
  const cfnFunction = new CfnFunction(stack, ..., {
    role: cfnRole,    // <-- I want to see this compile
  });

  Template.fromStack(stack).hasResourceProperties('AWS::Lambda::Function', { 
    Role: { Ref: ... }    // <-- and this produce the right template
  });
});

Can be any two resources that show up in the relationship data, pick your fave.

@leonmk-aws leonmk-aws force-pushed the leonmk-harmony branch 5 times, most recently from 1d3b416 to f2b1a2f Compare October 21, 2025 07:28
@leonmk-aws
Copy link
Contributor Author

Added unit tests in the lambda construct, testing unions, array of unions and nested relationships, as well as code to ensure arn properties appear first in the generated code: 02a8656

@gasolima
Copy link
Contributor

LGTM, I looked at the generated file and checked that the changes is ok. But didn't check if those relationship are right or not, since the only way to do it is to deploy stacks which unfeasible.

@leonmk-aws leonmk-aws removed the pr/do-not-merge This PR should not be merged at this time. label Oct 22, 2025
expect(fn.node.metadata).toStrictEqual([]);
});
});
describe('L1 Relationships', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think having a separate file for L1 things is better idea than putting everything here. Just to make it separate for ppl to understand.

@mergify
Copy link
Contributor

mergify bot commented Oct 22, 2025

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 mergify bot added queued and removed dequeued labels Oct 23, 2025
@mergify
Copy link
Contributor

mergify bot commented Oct 23, 2025

This pull request has been removed from the queue for the following reason: pull request dequeued.

Pull request #35713 has been dequeued. Mergify failed to merge the pull request. GitHub can't merge the pull request after 14 retries.
Waiting for the branch protection required status checks to be validated.

You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it.
If you do update this pull request, it will automatically be requeued once the queue conditions match again.
If you think this was a flaky issue instead, you can requeue the pull request, without updating it, by posting a @mergifyio requeue comment.

@mergify mergify bot added dequeued and removed queued labels Oct 23, 2025
@mergify
Copy link
Contributor

mergify bot commented Oct 23, 2025

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

@mrgrain
Copy link
Contributor

mrgrain commented Oct 23, 2025

@Mergifyio refresh

@mergify
Copy link
Contributor

mergify bot commented Oct 23, 2025

refresh

✅ Pull request refreshed

@mergify mergify bot added queued and removed dequeued labels Oct 23, 2025
@mergify
Copy link
Contributor

mergify bot commented Oct 23, 2025

This pull request has been removed from the queue for the following reason: pull request dequeued.

Pull request #35713 has been dequeued. Mergify failed to merge the pull request. GitHub can't merge the pull request after 14 retries.
Waiting for the branch protection required status checks to be validated.

You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it.
If you do update this pull request, it will automatically be requeued once the queue conditions match again.
If you think this was a flaky issue instead, you can requeue the pull request, without updating it, by posting a @mergifyio requeue comment.

@mergify mergify bot added dequeued and removed queued labels Oct 23, 2025
@mergify
Copy link
Contributor

mergify bot commented Oct 23, 2025

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

@mrgrain mrgrain added queued and removed dequeued labels Oct 23, 2025
@mergify mergify bot added dequeued and removed queued labels Oct 23, 2025
@mergify
Copy link
Contributor

mergify bot commented Oct 23, 2025

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

@mrgrain mrgrain removed the dequeued label Oct 23, 2025
@mergify mergify bot added the dequeued label Oct 23, 2025
@mergify
Copy link
Contributor

mergify bot commented Oct 23, 2025

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

@leonmk-aws
Copy link
Contributor Author

@Mergifyio refresh

@mergify
Copy link
Contributor

mergify bot commented Oct 23, 2025

refresh

✅ Pull request refreshed

@mergify
Copy link
Contributor

mergify bot commented Oct 23, 2025

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 mergify bot merged commit c5e7f21 into aws:main Oct 23, 2025
18 of 19 checks passed
@github-actions
Copy link
Contributor

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 23, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

contribution/core This is a PR that came from AWS. dequeued p2 pr-linter/exempt-integ-test The PR linter will not require integ test changes pr-linter/exempt-readme The PR linter will not require README changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants