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

fix(cloudformation-diff): Fix aws-sdk dependency issue #28680

Merged
merged 11 commits into from
Jan 16, 2024

Conversation

frankpengau
Copy link
Contributor

Missing dependency when exporting @aws-cdk/cloudformation-diff in custom AWS Construct Library.

Closes #28679


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

@aws-cdk-automation aws-cdk-automation requested a review from a team January 12, 2024 03:20
@github-actions github-actions bot added bug This issue is a bug. p2 beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK labels Jan 12, 2024
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.

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

@frankpengau frankpengau changed the title cloudformation-diff: Moved aws-sdk from devDependencies to dependencies fix(cloudformation-diff): Moved aws-sdk from devDependencies to dependencies Jan 12, 2024
latest version of aws-sdk
@frankpengau
Copy link
Contributor Author

Exemption Request

@aws-cdk-automation aws-cdk-automation added the pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. label Jan 12, 2024
Copy link
Contributor

@laurelmay laurelmay left a comment

Choose a reason for hiding this comment

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

Can the underlying issue be fixed instead by changing the offending import from

import { CloudFormation } from 'aws-sdk';

to

import type { CloudFormation } from 'aws-sdk';

The only reason the import is being is used is to get the type not to make any actual API calls. Changing the import should help make that clearer and prevent accidentally needing the aws-sdk at runtime.

In this case, the eslint-disable comment can be removed too since it's no longer an extraneous import.

@comcalvi
Copy link
Contributor

comcalvi commented Jan 12, 2024

Thanks for investigating @kylelaker, I just confirmed that your fix works on the reproduction repo. @frankpengau can you please confirm if using import type fixes the issue for you and if so update the PR accordingly?

@frankpengau frankpengau changed the title fix(cloudformation-diff): Moved aws-sdk from devDependencies to dependencies fix(cloudformation-diff): Fix aws-sdk deperndency issue Jan 13, 2024
@frankpengau frankpengau changed the title fix(cloudformation-diff): Fix aws-sdk deperndency issue fix(cloudformation-diff): Fix aws-sdk dependency issue Jan 13, 2024
@github-actions github-actions bot added effort/medium Medium work item – several days of effort p1 and removed p2 labels Jan 13, 2024
Copy link
Contributor

@laurelmay laurelmay left a comment

Choose a reason for hiding this comment

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

Thank you so much for this fix and for adjusting your PR based on the review feedback! This will definitely be a helpful change. Great catch!

@aws-cdk-automation aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Jan 13, 2024
@frankpengau
Copy link
Contributor Author

Exemption Request

@frankpengau
Copy link
Contributor Author

@kylelaker @comcalvi Is there anything else required from my end to get this change in?

@comcalvi comcalvi added pr-linter/exempt-readme The PR linter will not require README changes pr-linter/exempt-test The PR linter will not require test changes pr-linter/exempt-integ-test The PR linter will not require integ test changes and removed pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. labels Jan 16, 2024
@aws-cdk-automation aws-cdk-automation dismissed stale reviews from themself January 16, 2024 20:00

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

@aws-cdk-automation aws-cdk-automation removed pr/needs-maintainer-review This PR needs a review from a Core Team Member labels Jan 16, 2024
Copy link
Contributor

mergify bot commented Jan 16, 2024

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-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: 21ad562
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify mergify bot merged commit 109b2ab into aws:main Jan 16, 2024
9 checks passed
Copy link
Contributor

mergify bot commented Jan 16, 2024

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 bot pushed a commit that referenced this pull request Mar 16, 2024
…get CFN types resolved in exports (#28768)

Issue still persists after #28680. 
```
node_modules/@aws-cdk/cloudformation-diff/lib/diff-template.d.ts:1:37 - error TS2307: Cannot find module 'aws-sdk' or its corresponding type declarations.

1 import type { CloudFormation } from 'aws-sdk';
                                      ~~~~~~~~~


Found 1 error in node_modules/@aws-cdk/cloudformation-diff/lib/diff-template.d.ts:1
```

Types are still required as a direct dependency in package.json.

Closes #28679 

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
ahammond pushed a commit to ahammond/aws-cdk that referenced this pull request Mar 26, 2024
…get CFN types resolved in exports (aws#28768)

Issue still persists after aws#28680. 
```
node_modules/@aws-cdk/cloudformation-diff/lib/diff-template.d.ts:1:37 - error TS2307: Cannot find module 'aws-sdk' or its corresponding type declarations.

1 import type { CloudFormation } from 'aws-sdk';
                                      ~~~~~~~~~


Found 1 error in node_modules/@aws-cdk/cloudformation-diff/lib/diff-template.d.ts:1
```

Types are still required as a direct dependency in package.json.

Closes aws#28679 

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK bug This issue is a bug. effort/medium Medium work item – several days of effort p1 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 pr-linter/exempt-test The PR linter will not require test changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cloudformation-diff: Hidden dependency on aws-sdk
4 participants