Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Simplify props for GuUserData #478

Closed
wants to merge 1 commit into from

Conversation

akash1810
Copy link
Member

What does this change?

Following from #455, simplify the props for GuUserData by removing the bucket key in favour of using the GuDistributionBucketParameter singleton.

BREAKING CHANGE: Shape change to the props for GuUserData

The bucket field has been removed.

Does this change require changes to existing projects or CDK CLI?

Yes - the props to GuUserData has changed.

How to test

Tests continue to pass.

How can we measure success?

A simpler, more consistent API.

Have we considered potential risks?

With this change, we require an account to have the parameter store entry created. We do not have any tooling to assess an account's GuCDK readiness, so we're somewhat relying on our documentation for now.

@akash1810 akash1810 requested a review from a team April 19, 2021 21:32
@akash1810 akash1810 changed the title feat: Simplify props for GuUserData feat: Simplify props for GuUserData Apr 19, 2021
@@ -11,7 +12,6 @@ import type { AppIdentity } from "../core/identity";
* `executionStatement` will be something like "dpkg -i application.deb` or `service foo start`.
*/
export interface GuUserDataS3DistributableProps {
bucket: GuDistributionBucketParameter;
fileName: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

It's probably worth mentioning where the bucket name is coming from in this comment (i.e. something similar to this one). Tangential, but this makes me wonder if we should fix the key/fileName inconsistency too?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's probably worth mentioning where the bucket name is coming from in this comment

👍🏽 thanks for the reminder!

Tangential, but this makes me wonder if we should fix the key/fileName inconsistency too?

Absolutely! Spoiler alert: this is going to be raised in a couple of PRs 😄 .

Copy link
Contributor

Choose a reason for hiding this comment

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

I thought you'd probably notice it too! 😁

Following from #455, simplify the props for `GuUserData` by removing the bucket key in favour of using the `GuDistributionBucketParameter` singleton.

BREAKING CHANGE: Shape change to the props for `GuUserData`

The `bucket` field has been removed.
@akash1810
Copy link
Member Author

Closing in favour of #479 which provides a change that can more easily be added to our lambdas.

@akash1810 akash1810 closed this Apr 20, 2021
@akash1810 akash1810 deleted the aa-static-user-data-bucket branch April 20, 2021 12:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants