feat(servicecatalogappregistry): add sharing of applications and attribute groups #20850
Conversation
|
|
|
need update table of contents. |
packages/@aws-cdk/aws-servicecatalogappregistry/test/integ.application.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-servicecatalogappregistry/test/integ.attribute-group.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-servicecatalogappregistry/lib/application.ts
Outdated
Show resolved
Hide resolved
| public shareResource(shareOptions: ShareOptions): void { | ||
| const principals = getPrincipalsforSharing(shareOptions); | ||
| const shareName = `RAMShare${hashValues(this.node.addr, ...principals)}`; | ||
| new CfnResourceShare(this, shareName, { |
There was a problem hiding this comment.
We could consider since we have hash for resource name we can either throw an error if someone tries to add same share twice or ignore it, like we do in other SC resources.
There was a problem hiding this comment.
I will think about this one a bit more. I think some logic to ignore it may be sufficient.
| } | ||
|
|
||
| /** | ||
| * Generates a unique hash identfifer using SHA256 encryption algorithm |
There was a problem hiding this comment.
| * Generates a unique hash identfifer using SHA256 encryption algorithm | |
| * Generates a unique hash identfifer using SHA256 encryption algorithm. |
mackalex
force-pushed
the
appreg_ram_share
branch
from
1b5c36e
to
c785e58
Compare
packages/@aws-cdk/aws-servicecatalogappregistry/lib/attribute-group.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-servicecatalogappregistry/test/integ.attribute-group.ts
Outdated
Show resolved
Hide resolved
| * | ||
| * @default true | ||
| */ | ||
| readonly allowExternalPrincipals?: boolean; |
There was a problem hiding this comment.
not sure if this should be true be default...pinging @rix0rrr for advice
There was a problem hiding this comment.
Depends on what this flag does.
- As currently stated, it automatically shares your app with everyone. That doesn't seem great... what's the thought behind that @arcrank ?
- If the actual behavior is, this turns on the possibility of calling some other method later which shares, then it's fine, but also the documentation is completely failing to explain that.
Also, the docstring says "Explicitly" but proceeds to turn on the flag implicitly, which makes that word pretty redundant.
I'd suggest rewriting the docstring to be clear and specific and actionable.
There was a problem hiding this comment.
This is default behavior, all it means is that you can share with accounts not in your AWS Org. Since this is really AWS Org specific (this flag is meaningless if the caller account itself is not in an aws org) maybe we can rename it align with that.
There was a problem hiding this comment.
I revised the docstring to be more clear on this. +1 to @arcrank on this. The default behavior is explained further in step 8 of RAM's resource sharing getting started guide: https://docs.aws.amazon.com/ram/latest/userguide/getting-started-sharing.html
There was a problem hiding this comment.
I would further explain the behavior in these docs to address @rix0rrr's second point; is there a method that must be called to actually share this resource? This docstring should tell the user not only that this sharing can happen, but how it happens.
There was a problem hiding this comment.
If the behavior relates to the organization, why don't we call it allowPrincipalsOutsideOrganization ?
There was a problem hiding this comment.
Thanks for the discussion on this. After conferring with the team, this property has been removed from ShareOptions as it is always set to false in AppRegistry's sharing experience with RAM and not surfaced to the customer.
packages/@aws-cdk/aws-servicecatalogappregistry/lib/application.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-servicecatalogappregistry/lib/attribute-group.ts
Outdated
Show resolved
Hide resolved
…group.ts Co-authored-by: Calvin Combs <66279577+comcalvi@users.noreply.github.com>
…n.ts Co-authored-by: Calvin Combs <66279577+comcalvi@users.noreply.github.com>
packages/@aws-cdk/aws-servicecatalogappregistry/test/attribute-group.test.ts
Show resolved
Hide resolved
TheRealAmazonKendra
changed the title
|
@mackalex When you merge from live, please use |
@TheRealAmazonKendra Hi Kendra, I'm looking for an additional approval or signoff on the requested changes so I can go ahead and merge this in. Thanks in advance! |
|
Apologies for neglecting this one. I'll take another look on Monday. |
|
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). |
|
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). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
…ibute groups (aws#20850) This PR adds sharing capability to the Application and Attribute Group constructs for Service Catalog AppRegistry. Users who have enabled AWS Organizations in their AWS account can now share their AppRegistry Application and Attribute Groups with accounts in their organization, organizational units (OUs), IAM roles, and IAM users. This provides CDK parity with the support of cross-account sharing of Applications and Attribute Groups which was [released as an AppRegistry feature](https://aws.amazon.com/about-aws/whats-new/2022/06/aws-service-catalogs-application-registry-cross-account-applications/). ---- ### All Submissions: * [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [x] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [x] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* --- Co-authored by: Aidan Crank
This PR adds sharing capability to the Application and Attribute Group constructs for Service Catalog AppRegistry. Users who have enabled AWS Organizations in their AWS account can now share their AppRegistry Application and Attribute Groups with accounts in their organization, organizational units (OUs), IAM roles, and IAM users. This provides CDK parity with the support of cross-account sharing of Applications and Attribute Groups which was released as an AppRegistry feature.
All Submissions:
Adding new Unconventional Dependencies:
New Features
yarn integto deploy the infrastructure and generate the snapshot (i.e.yarn integwithout--dry-run)?By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license
Co-authored by: Aidan Crank