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: stops cdk v1 warning for non cdk custom resource #12241

Merged
merged 2 commits into from
Mar 16, 2023

Conversation

akshbhu
Copy link
Contributor

@akshbhu akshbhu commented Mar 15, 2023

Description of changes

  • added a check to stop cdk v1 warning for non cdk custom resource

Issue #, if available

Description of how you validated changes

  • manually tested
  • updated e2e to check for specific issue

Checklist

  • PR description included
  • yarn test passes
  • Tests are changed or added
  • Relevant documentation is changed or added (and PR referenced)
  • New AWS SDK calls or CloudFormation actions have been added to relevant test and service IAM policies
  • Pull request labels are added

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

@akshbhu akshbhu requested a review from a team as a code owner March 15, 2023 22:31
sobolk
sobolk previously approved these changes Mar 15, 2023
@@ -65,7 +65,7 @@ const getOverridesWarning = (resourcesToBuild: IAmplifyResource[], dependencyToS
const getCustomResourcesWarning = (resourcesToBuild: IAmplifyResource[], dependencyToSearch: string): AmplifyWarning | undefined => {
let customResourcesWarningObject;
const customResourceImpactedFiles = [];
const customCategoryResources = resourcesToBuild.filter((resource) => resource.category === AmplifyCategories.CUSTOM);
const customCategoryResources = resourcesToBuild.filter((resource) => resource.category === AmplifyCategories.CUSTOM && resource.service !== 'customCloudformation');
Copy link
Member

@sobolk sobolk Mar 15, 2023

Choose a reason for hiding this comment

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

consider extracting second part of condition to !isCFNBasedCustomeResource(resource) for future readers.

Comment on lines 42 to 46
await addCFNCustomResource(projRoot, { name: cfnResourceNameWithV10, promptForCategorySelection: false });
const srcCFNCustomResourceFilePath = path.join(__dirname, '..', '..', '..', 'custom-resources', 'custom-cfn-stack.json');
// adding a resource to custom cfn stack
const destCFNCustomResourceFilePath = path.join(projRoot, 'amplify', 'backend', 'custom', cfnResourceNameWithV10,`${cfnResourceNameWithV10}-cloudformation-template.json`);
fs.copyFileSync(srcCFNCustomResourceFilePath, destCFNCustomResourceFilePath);
Copy link
Member

Choose a reason for hiding this comment

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

consider adding one non-migration test that would spin up only CFN based custom without any CDK.

@codecov-commenter
Copy link

Codecov Report

Merging #12241 (4f04312) into dev (984c0f8) will decrease coverage by 2.30%.
The diff coverage is 55.23%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##              dev   #12241      +/-   ##
==========================================
- Coverage   49.45%   47.15%   -2.30%     
==========================================
  Files         692      836     +144     
  Lines       33424    38038    +4614     
  Branches     6840     7715     +875     
==========================================
+ Hits        16529    17937    +1408     
- Misses      15370    18465    +3095     
- Partials     1525     1636     +111     
Impacted Files Coverage Δ
...mplify-appsync-simulator/src/resolvers/function.ts 15.38% <ø> (ø)
...s/amplify-appsync-simulator/src/type-definition.ts 100.00% <ø> (ø)
...sync-simulator/src/velocity/util/dynamodb-utils.ts 6.55% <0.00%> (ø)
...y-appsync-simulator/src/velocity/util/map-utils.ts 10.00% <ø> (ø)
...tor/src/velocity/util/transform/dynamodb-filter.ts 4.93% <0.00%> (ø)
...appsync-simulator/src/velocity/value-mapper/map.ts 86.88% <ø> (ø)
...ackages/amplify-category-auth/src/commands/auth.ts 0.00% <0.00%> (ø)
...es/amplify-category-auth/src/commands/auth/help.ts 0.00% <0.00%> (ø)
.../amplify-category-auth/src/commands/auth/import.ts 0.00% <0.00%> (ø)
...dformation/auth-inputs-manager/auth-input-state.ts 46.93% <0.00%> (ø)
... and 105 more

... and 456 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@sobolk sobolk merged commit 6a3d08c into aws-amplify:dev Mar 16, 2023
sobolk pushed a commit to sobolk/amplify-cli that referenced this pull request Mar 16, 2023
)

* fix: stops cdk warning for non cdk custom resource

* fix: added unit test

---------

Co-authored-by: Akshay Upadhyay <akz@amazon.com>
sobolk added a commit that referenced this pull request Mar 16, 2023
* fix: stops cdk warning for non cdk custom resource

* fix: added unit test

---------

Co-authored-by: akshbhu <39866697+akshbhu@users.noreply.github.com>
Co-authored-by: Akshay Upadhyay <akz@amazon.com>
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.

None yet

4 participants