feat: QuickSightSubscription construct #505
Conversation
lmouhib
changed the title
|
|
||
|
|
||
| /** | ||
| * The method that you want to use to authenticate your Amazon QuickSight account. |
| export interface QuickSightSubscriptionProps { | ||
|
|
||
| /** | ||
| * The name of your Amazon QuickSight account. This name is unique over all of Amazon Web Services, and it appears only when users sign in. |
There was a problem hiding this comment.
nit, can you break the line here to make read easier?
| * The edition of Amazon QuickSight that you want your account to have. Currently, you can choose from ENTERPRISE or ENTERPRISE_AND_Q . | ||
| * @default - ENTERPRISE is used as default. | ||
| */ | ||
| readonly edition: string; |
There was a problem hiding this comment.
Make this an enum, to avoid bad input.
| this.executionRole = new Role(this, 'Role', { | ||
| assumedBy: new ServicePrincipal('lambda.amazonaws.com'), | ||
| managedPolicies: [ | ||
| ManagedPolicy.fromAwsManagedPolicyName('service-role/AWSLambdaVPCAccessExecutionRole'), |
There was a problem hiding this comment.
We avoid using managed roles as much as possible, can you check the MSK PR for how to build this policy?
There was a problem hiding this comment.
This is the LambdaExecution role that as we clarified is already added by the CR Provider so will remove it, I am adding QS specific policies as an inline policy (see lines 120-173 on quicksight-subscription.ts) . Let me know if this is ok or shall we use customer managed policies for this.
| }, | ||
| }); | ||
|
|
||
| const timeout = props.executionTimeout || Duration.minutes(5); |
There was a problem hiding this comment.
can you make this check null and not false. meaning ?? instead of ||.
| IDENTITY_REGION: props.identityRegion, | ||
| }, | ||
| }, | ||
| queryInterval: Duration.seconds(1), |
There was a problem hiding this comment.
isn't this too aggressive?
lmouhib
force-pushed
the
feature/quicksight
branch
from
32e5734
to
93f0833
Compare
|
|
||
| /** | ||
| * The name of your Amazon QuickSight account. | ||
| * This name is unique over all of Amazon Web Services, and it appears only when users sign in. |
| /** | ||
| * The method that you want to use to authenticate your Amazon QuickSight account. | ||
| * Only IAM_IDENTITY_CENTER, IAM_AND_QUICKSIGHT and IAM_ONLY are supported | ||
| * @default |
| /** | ||
| * The Amazon Web Services account ID of the account that you're using to create your Amazon QuickSight account. | ||
| */ | ||
| readonly awsAccountId: string; |
There was a problem hiding this comment.
what's the benefit of configuring a different account than the CDK one?
There was a problem hiding this comment.
mm, this could be benefitial to support the creation of pipelines if we want to support them in the future:
For me the CDK account would be the one used for the development account below in the architecture and then the user would be able to create constants for DEV, PRE, PRO (or any additional stages) they would want to configure. Does it make sense ?
| } | ||
|
|
||
| export enum QuickSightEdition { | ||
| ENTERPRISE = 'ENTERPRISE', |
There was a problem hiding this comment.
can we add another option?
There was a problem hiding this comment.
we can add the ENTERPRISE_AND_Q (as standard is rarely used and also won't support templates, or assets as bundle functionality so will break the support to pipelines once we support them).
The only thing with supporting ENTERPRISE_AND_Q is that we need to make aware the customer of the 250$/month fee of enabling Q in the account (irrelevant on whether they use it or no)
| /** | ||
| * | ||
| */ | ||
| public static readonly RESOURCE_TYPE = 'Custom::QuickSightSubscription'; |
There was a problem hiding this comment.
Should be private or directly stored in the constructor
There was a problem hiding this comment.
will move it to the constructor along with the custom resource creation
| /** | ||
| * The email address that you want Amazon QuickSight to send notifications to regarding your Amazon QuickSight account or Amazon QuickSight subscription. | ||
| */ | ||
| readonly notificationEmail: string; |
There was a problem hiding this comment.
should it be private? same for the following members?
| } | ||
|
|
||
|
|
||
| public createQuickSightSubscription() { |
There was a problem hiding this comment.
can you bundle the custom resource directly in the constructor?
Issue #, if available:
Description of changes:
Initial version of the QuickSight construct (QS Subscription functionality using custom resource) for review
Checklist
fix:,feat:,docs:, ...)By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.