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

DO-1530: Upgrade constructs #1073

Merged
merged 10 commits into from
Sep 29, 2023

Conversation

gowrizrh
Copy link
Contributor

@gowrizrh gowrizrh commented Sep 27, 2023

Description of the proposed changes

  • Create new Esbuild package
  • Upgrade basic-auth construct to CDK v2
  • Upgrade cloudfront-security-headers to CDK v2
  • Upgrade geoip-redirect to CDK v2
  • Update dependencies of lambda-at-edge-handlers
  • Upgrade prerender-proxy to CDK v2
  • Upgrade rabbitmq to CDK v2
  • Upgrade shared-vpc to CDK v2
  • Upgrade waf to CDK v2

@gowrizrh gowrizrh changed the base branch from main to epic/cdk-v2 September 27, 2023 02:03
@gowrizrh gowrizrh marked this pull request as ready for review September 27, 2023 02:04
* upgrade basic-auth construct
* upgrade cloudfront-security-headers construct
@gowrizrh gowrizrh changed the title DO-1534: Upgrade rabbitmq construct DO-1530: Upgrade constructs Sep 27, 2023
@gowrizrh gowrizrh marked this pull request as draft September 27, 2023 04:29
@gowrizrh gowrizrh marked this pull request as ready for review September 27, 2023 05:25
Copy link
Contributor

@TheOrangePuff TheOrangePuff left a comment

Choose a reason for hiding this comment

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

LGTM

"build": "",
"prepublish": "",
"build": "tsc",
"prepublish": "tsc",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why tsc on prepublish? 🤔 Just thinking in the pipeline we'd probably build and then publish so this would run tsc twice. If we're just gonna run publish I think changing from tsc to npm run build makes more sense

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this currently exists in the main branch. i only removed the commands for convenience and then re-added them back 🤔

) {
super(scope, id);

const defineOptions: any = {};
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a type for this rather than any?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i think we could add __CONTENT_SECURITY_POLICY__ 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to include this if it's all commented out? 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

probably not, i'm not sure if this was even right, but i didn't spend anytime migrating tests 😛

@gowrizrh
Copy link
Contributor Author

thanks @TheOrangePuff, i'll make the suggested changes in a separate PR

@gowrizrh gowrizrh merged commit aef577f into epic/cdk-v2 Sep 29, 2023
1 check passed
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