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

Cross account support for V2 function transformer and directive #975

Merged
merged 7 commits into from
Nov 14, 2022

Conversation

phani-srikar
Copy link
Contributor

@phani-srikar phani-srikar commented Nov 9, 2022

Description of changes

This PR is an extension of #639 with the following changes:

  • Prevent circular dependency between transformer common and transformer core.
  • Remove changes to V1 transformers
  • Added e2e tests for V2 function transformer changes

In Summary, with these changes, customers can use the newly added accountId input to @function directive to attach lambda functions that are maintained in different AWS account and are managed independently.

Issue #, if available

Description of how you validated changes

Unit testing
Added e2e tests
In a JS sample app

Full e2e pipeline run

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

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

@phani-srikar phani-srikar requested a review from a team as a code owner November 9, 2022 17:01
@phani-srikar phani-srikar marked this pull request as draft November 9, 2022 17:01
@lgtm-com
Copy link

lgtm-com bot commented Nov 9, 2022

This pull request fixes 1 alert when merging f773765 into a928809 - view on LGTM.com

fixed alerts:

  • 1 for Unused variable, import, function or class

alharris-at
alharris-at previously approved these changes Nov 9, 2022
@lgtm-com
Copy link

lgtm-com bot commented Nov 10, 2022

This pull request fixes 1 alert when merging dc8b9f1 into a928809 - view on LGTM.com

fixed alerts:

  • 1 for Unused variable, import, function or class

philipmw and others added 3 commits November 10, 2022 06:11
…lvers using @function (#499)

This change does not come with E2E tests because I don't know whether the
existing CI infra has access to multiple AWS accounts.

However, I tested this in my own AWS accounts and verified that the
AppSync Data Source points to a custom AWS account ID.
@phani-srikar phani-srikar force-pushed the cross-account-function-directive branch from dc8b9f1 to 96f32b5 Compare November 10, 2022 14:12
@lgtm-com
Copy link

lgtm-com bot commented Nov 10, 2022

This pull request fixes 1 alert when merging 96f32b5 into a928809 - view on LGTM.com

fixed alerts:

  • 1 for Unused variable, import, function or class

@phani-srikar phani-srikar marked this pull request as ready for review November 10, 2022 18:37
@phani-srikar phani-srikar requested review from alharris-at and a team November 10, 2022 18:37
@phani-srikar phani-srikar changed the title Cross account function directive Cross account support for V2 function transformer and directive Nov 10, 2022
alharris-at
alharris-at previously approved these changes Nov 10, 2022
AaronZyLee
AaronZyLee previously approved these changes Nov 11, 2022
Copy link
Contributor

@AaronZyLee AaronZyLee left a comment

Choose a reason for hiding this comment

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

Left some comments below. Otherwise LGTM

@codecov-commenter
Copy link

Codecov Report

Merging #975 (7f07935) into main (6a7b9f3) will increase coverage by 0.17%.
The diff coverage is 56.98%.

@@            Coverage Diff             @@
##             main     #975      +/-   ##
==========================================
+ Coverage   63.14%   63.32%   +0.17%     
==========================================
  Files         278      282       +4     
  Lines       18125    18376     +251     
  Branches     4375     4460      +85     
==========================================
+ Hits        11445    11636     +191     
- Misses       6093     6130      +37     
- Partials      587      610      +23     
Impacted Files Coverage Δ
...udformation/utils/migrate-api-override-resource.ts 64.28% <0.00%> (ø)
...-graphql-auth-transformer/src/utils/definitions.ts 100.00% <ø> (ø)
...l-http-transformer/src/graphql-http-transformer.ts 96.21% <ø> (ø)
...-transformer/src/graphql-belongs-to-transformer.ts 97.26% <ø> (ø)
...al-transformer/src/graphql-has-many-transformer.ts 100.00% <ø> (ø)
...nal-transformer/src/graphql-has-one-transformer.ts 98.59% <ø> (ø)
...ransformer/src/graphql-many-to-many-transformer.ts 97.73% <ø> (ø)
...fy-graphql-relational-transformer/src/resolvers.ts 97.50% <ø> (+0.53%) ⬆️
...plify-graphql-relational-transformer/src/schema.ts 93.49% <ø> (-0.36%) ⬇️
...mplify-graphql-relational-transformer/src/utils.ts 79.24% <ø> (-2.17%) ⬇️
... and 83 more

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

AaronZyLee
AaronZyLee previously approved these changes Nov 11, 2022
alharris-at
alharris-at previously approved these changes Nov 14, 2022
@@ -2,12 +2,12 @@ import { simplifyName } from './util';
import md5 from 'md5';

export class FunctionResourceIDs {
static FunctionDataSourceID(name: string, region?: string): string {
return `${simplifyName(name)}${simplifyName(region || '')}LambdaDataSource`;
static FunctionDataSourceID(name: string, region?: string, accountId?:string): string {
Copy link
Contributor

Choose a reason for hiding this comment

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

n.p. on spacing after the colon here, and in the next few functions as well.

@alharris-at alharris-at merged commit 06a7aab into main Nov 14, 2022
@alharris-at alharris-at deleted the cross-account-function-directive branch November 14, 2022 19:26
@phani-srikar phani-srikar mentioned this pull request Dec 2, 2022
5 tasks
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.

5 participants