Allow 3rd-party plugin to CDK override #9601

fossamagna · 2022-01-25T13:51:48Z

Description of changes

Allow 3rd-party plugin to CDK override. Some restricted points to starts with @aws-amplify/amplify-category- literal changed to use package location in pluginInfo.

Issue #, if available

fix #9226

Description of how you validated changes

Unitt Testing
Manual Testing (using 3rd-party plugin)
1. Setup Local Enviroment
  1. Run git clone -b allow-3rd-party-override git@github.com:fossamagna/amplify-cli.git
  2. Setup local environment
    1. Run cd amlify-cli
    2. Run yarn setup-dev
  3. Rungit clone -b override git@github.com:fossamagna/amplify-category-console-notification.git
  4. Run cd amplify-category-console-notification
  5. Change version of amplify-cli-core in package.json to the path to local amplify-cli-core you checked out in the previous step.
  6. Run yarn build
  7. Run amplify-dev plugin add $(pwd)/amplify-category-console-notification
  8. Run amplify-dev plugin scan
2. Run amplify-dev init. initialize sample project in order to test add command.
3. Run amplify-dev add consolenotification

Checklist

PR description included
yarn test passes
Tests are changed or added

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

codecov-commenter · 2022-01-26T00:35:14Z

Codecov Report

Merging #9601 (652e404) into master (896b518) will increase coverage by 0.10%.
The diff coverage is 37.93%.

@@            Coverage Diff             @@
##           master    #9601      +/-   ##
==========================================
+ Coverage   46.67%   46.78%   +0.10%     
==========================================
  Files         706      708       +2     
  Lines       35268    35273       +5     
  Branches     7148     7149       +1     
==========================================
+ Hits        16462    16502      +40     
+ Misses      17014    16981      -33     
+ Partials     1792     1790       -2

Impacted Files	Coverage Δ
...graphql-transformer/transform-graphql-schema-v2.ts	`14.87% <ø> (ø)`
...vider-utils/awscloudformation/apigw-input-state.ts	`88.00% <0.00%> (ø)`
...loudformation/utils/check-appsync-api-migration.ts	`35.71% <0.00%> (ø)`
...udformation/utils/attach-prev-params-to-context.ts	`50.00% <0.00%> (ø)`
...wscloudformation/utils/auth-sms-workflow-helper.ts	`43.75% <0.00%> (ø)`
...ation/service-walkthroughs/dynamoDB-input-state.ts	`7.35% <0.00%> (ø)`
...ls/awscloudformation/handlers/resource-handlers.ts	`14.14% <5.12%> (-0.72%)`	⬇️
...mation/service-walkthroughs/s3-user-input-state.ts	`18.79% <13.04%> (ø)`
...dformation/cdk-stack-builder/s3-stack-transform.ts	`64.74% <16.66%> (ø)`
packages/amplify-category-storage/src/index.ts	`23.44% <25.00%> (ø)`
... and 20 more

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

fossamagna · 2022-02-07T05:05:10Z

@akshbhu @ammarkarachi @jhockett @kaustavghosh06
Is this RP has blockers? Is there anything I can do to get my PR reviewed?
Thanks.

jhockett · 2022-03-14T19:28:55Z

Hey @fossamagna, sorry for the delayed response.

One of our team's internal goals is to reduce the usage of the context object because it makes the code more susceptible to side effects. What is the use case for the changes made to auth, storage, and api?

jhockett · 2022-03-14T19:16:52Z

packages/amplify-provider-awscloudformation/src/override-manager/transform-resource.ts

+      const pluginInfo = context.amplify.getCategoryPluginInfo(context, resource.category, resource.resourceName);
+      const { transformCategoryStack } = pluginInfo ? await import(pluginInfo.packageLocation) : { transformCategoryStack: null };


If this PR was reduced to these lines here, I think that would achieve the majority of use cases for #9226. Thoughts @fossamagna?

Hi, @jhockett

If this PR was reduced to these lines here, I think that would achieve the majority of use cases for #9226.

Yes, I think so.

What is the use case for the changes made to auth, storage, and api?

I added context object to use CLIInputSchemaValidator on 3rd-party plugin at following.

https://github.com/aws-amplify/amplify-cli/pull/9601/files#diff-0261ff0abc6d6facc760fc5319008e2ebe0993de83bd0a2898a113c526a16372R149

fossamagna/amplify-category-console-notification@961262f#diff-be6013127ca9dce2788dc277cc0fefa77620c5aee6a7ec82fcb41462b415c6d4R42

I think that it will be resolved by implementing equivalent CLIInputSchemaValidator in 3rd-party plugin.

I understand now, thanks for the examples! I think we should include the plugin support for the schema generator too.

@jhockett
I think that it is unnecessary modify to support for the schema generator, because generated schema path resolved by relative path. If I overlooking what should be support for the schema generator, I would like to seek your advice on this.

amplify-cli/packages/amplify-cli-core/src/category-interfaces/category-base-schema-generator.ts

Lines 32 to 33 in ae7b001

TYPES_SRC_ROOT = path.join('.', 'src', 'provider-utils', 'awscloudformation', 'service-walkthrough-types');

SCHEMA_FILES_ROOT = path.join('.', 'resources', 'schemas');

@fossamagna The schema generator takes a category's TypeScript type for the input state and returns a JSON schema that the category's cli-inputs.json should conform to.

Example input state schema for REST APIs: https://github.com/aws-amplify/amplify-cli/blob/ae7b001f274f327a29c99c67fe851272c6208e84/packages/amplify-category-api/resources/schemas/aPIGateway/APIGatewayCLIInputs.schema.json

Where the schema validation logic is invoked:

amplify-cli/packages/amplify-category-api/src/provider-utils/awscloudformation/apigw-input-state.ts

Line 217 in ae7b001

public isCLIInputsValid(cliInputs?: ApigwInputs) {

@jhockett I understand now, schema generator is pointing to CLIInputSchemaValidator class in category-base-schema-genereator.ts, not CLIInputSchemaGenerator class.
Then, I don't think I have any part of this PR that needs to be corrected.
What do you think on this?

Hey @fossamagna, sorry for the delayed response. Thinking about it more, this approach LGTM 👍

packages/amplify-category-api/src/index.ts

jhockett · 2022-03-24T18:20:37Z

packages/amplify-provider-awscloudformation/src/override-manager/transform-resource.ts

+      const pluginInfo = context.amplify.getCategoryPluginInfo(context, resource.category, resource.resourceName);
+      const { transformCategoryStack } = pluginInfo ? await import(pluginInfo.packageLocation) : { transformCategoryStack: null };


Hey @fossamagna, sorry for the delayed response. Thinking about it more, this approach LGTM 👍

jhockett

Overall LGTM @fossamagna! I have one requested change above regarding a removed export and I'd also like to see a unit test for custom plugins to make sure we avoid regressions here.

Thank you for the contribution!

fossamagna · 2022-03-25T09:13:57Z

@jhockett Thank you for the review many times.
I revert removed export and add unit tests for transformResourceWithOverrides and CLIInputSchemaValidator.

fossamagna · 2022-04-17T13:49:14Z

@jhockett Is this RP has blockers? Is there anything I can do?
Thanks.

jhockett · 2022-05-13T00:28:39Z

Apologies for the delayed review @fossamagna, LGTM

Thank you for your contribution!

ammarkarachi · 2022-05-27T22:30:28Z

@fossamagna Thanks for the PR this is good work, A PR got merged would need rebasing

fossamagna · 2022-05-28T00:40:37Z

@ammarkarachi Thank you for your review.
I resolved conflicts with git merge. Should I do git rebase?

ammarkarachi · 2022-05-28T00:49:39Z

It's good we are going to squash the commits anyway

path to schema json restricted to starts with `@aws-amplify/amplify-category-` literal, changed to use package location in pluginInfo as loading path of schema json ref aws-amplify#9226

…rceWithOverrides

fossamagna requested a review from a team as a code owner January 25, 2022 13:51

jhockett reviewed Mar 14, 2022

View reviewed changes

fossamagna requested a review from jhockett March 23, 2022 13:06

jhockett reviewed Mar 24, 2022

View reviewed changes

jhockett suggested changes Mar 24, 2022

View reviewed changes

fossamagna requested a review from jhockett March 25, 2022 09:14

fossamagna requested review from marcvberg, phani-srikar, sundersc and alharris-at as code owners May 10, 2022 00:53

jhockett approved these changes May 13, 2022

View reviewed changes

ammarkarachi approved these changes May 27, 2022

View reviewed changes

fossamagna added 6 commits May 28, 2022 09:54

refactor(cli): use plugin location instead of static module name prefix

358a825

feat: allow to validate 3rd-party cli-input.json schema

a02cf28

path to schema json restricted to starts with `@aws-amplify/amplify-category-` literal, changed to use package location in pluginInfo as loading path of schema json ref aws-amplify#9226

style: fix lint no trailing spaces error

8dcbce2

fix(amplify-category-api): apply suggestions from code review

016ae8a

test(amplify-provider-awscloudformation): add test for transformResou…

2ce29b3

…rceWithOverrides

test(amplify-cli-core): add test for CLIInputSchemaValidator

4c4a37d

fossamagna force-pushed the allow-3rd-party-override branch from 652e404 to 4c4a37d Compare May 28, 2022 01:23

jhockett merged commit 60498c5 into aws-amplify:master May 31, 2022

ykethan mentioned this pull request Jun 8, 2022

Adding Lambda function and pushing it displays override not implemented message #10546

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow 3rd-party plugin to CDK override #9601

Allow 3rd-party plugin to CDK override #9601

fossamagna commented Jan 25, 2022

codecov-commenter commented Jan 26, 2022 •

edited

fossamagna commented Feb 7, 2022

jhockett commented Mar 14, 2022

jhockett Mar 14, 2022

fossamagna Mar 15, 2022

jhockett Mar 15, 2022

fossamagna Mar 15, 2022

jhockett Mar 16, 2022 •

edited

fossamagna Mar 17, 2022

jhockett Mar 24, 2022

jhockett Mar 24, 2022

jhockett left a comment

fossamagna commented Mar 25, 2022 •

edited

fossamagna commented Apr 17, 2022

jhockett commented May 13, 2022 •

edited

ammarkarachi commented May 27, 2022 •

edited

fossamagna commented May 28, 2022

ammarkarachi commented May 28, 2022

		const pluginInfo = context.amplify.getCategoryPluginInfo(context, resource.category, resource.resourceName);
		const { transformCategoryStack } = pluginInfo ? await import(pluginInfo.packageLocation) : { transformCategoryStack: null };

	TYPES_SRC_ROOT = path.join('.', 'src', 'provider-utils', 'awscloudformation', 'service-walkthrough-types');
	SCHEMA_FILES_ROOT = path.join('.', 'resources', 'schemas');

Allow 3rd-party plugin to CDK override #9601

Allow 3rd-party plugin to CDK override #9601

Conversation

fossamagna commented Jan 25, 2022

Description of changes

Issue #, if available

Description of how you validated changes

Checklist

codecov-commenter commented Jan 26, 2022 • edited

Codecov Report

fossamagna commented Feb 7, 2022

jhockett commented Mar 14, 2022

jhockett Mar 14, 2022

Choose a reason for hiding this comment

fossamagna Mar 15, 2022

Choose a reason for hiding this comment

jhockett Mar 15, 2022

Choose a reason for hiding this comment

fossamagna Mar 15, 2022

Choose a reason for hiding this comment

jhockett Mar 16, 2022 • edited

Choose a reason for hiding this comment

fossamagna Mar 17, 2022

Choose a reason for hiding this comment

jhockett Mar 24, 2022

Choose a reason for hiding this comment

jhockett Mar 24, 2022

Choose a reason for hiding this comment

jhockett left a comment

Choose a reason for hiding this comment

fossamagna commented Mar 25, 2022 • edited

fossamagna commented Apr 17, 2022

jhockett commented May 13, 2022 • edited

ammarkarachi commented May 27, 2022 • edited

fossamagna commented May 28, 2022

ammarkarachi commented May 28, 2022

codecov-commenter commented Jan 26, 2022 •

edited

jhockett Mar 16, 2022 •

edited

fossamagna commented Mar 25, 2022 •

edited

jhockett commented May 13, 2022 •

edited

ammarkarachi commented May 27, 2022 •

edited