-
Notifications
You must be signed in to change notification settings - Fork 72
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
Deep directive merge #772
Deep directive merge #772
Conversation
This pull request introduces 2 alerts when merging b1368d7 into d7bf74d - view on LGTM.com new alerts:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two comments:
- I pulled down the branch and the build is failing because the def of
getArguments
acceptsoptions
but not FF provider and you're padding the later from several places. Please check build-test and also cloud e2e. - We should either accept the
options
or the FF provider (I prefer the first one) from changed methods in V2 core, to avoid 2 sources of truth. In order to make the construction of these options easier & centralized, you could create a util function in V2 core that accepts the FF provider and gives back the options. This util function is meant to be used from V2 transformers and not from CLI, so from CLI invocation, we will still pass in theoptions
.
@phani-srikar Running tests against this now, I thought I had fixes for these things in the other branch I tried to merge in locally but I guess the fixes didn't make it for some reason |
This pull request introduces 1 alert when merging 1131948 into d7bf74d - view on LGTM.com new alerts:
|
This pull request introduces 1 alert when merging d14eb82 into d7bf74d - view on LGTM.com new alerts:
|
This pull request introduces 1 alert when merging 216b04c into d7bf74d - view on LGTM.com new alerts:
|
packages/amplify-graphql-auth-transformer/src/graphql-auth-transformer.ts
Outdated
Show resolved
Hide resolved
4f064b1
to
6792e4e
Compare
This pull request introduces 1 alert when merging 6792e4e into 98c021a - view on LGTM.com new alerts:
|
This pull request fixes 1 alert when merging 6de7a4a into 98c021a - view on LGTM.com fixed alerts:
|
273871c
to
49ff008
Compare
This pull request fixes 1 alert when merging 49ff008 into 9ad36ac - view on LGTM.com fixed alerts:
|
49ff008
to
79f2ec1
Compare
This pull request fixes 1 alert when merging 79f2ec1 into c79232f - view on LGTM.com fixed alerts:
|
Clone default args before merge to prevent any accidental leakage between different calls Fix the isField value for object auth rules
This pull request fixes 1 alert when merging 47435d8 into c324083 - view on LGTM.com fixed alerts:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly looks good. Just called out few left out lib imports and a question.
packages/amplify-graphql-auth-transformer/src/utils/definitions.ts
Outdated
Show resolved
Hide resolved
packages/amplify-graphql-model-transformer/src/graphql-model-transformer.ts
Show resolved
Hide resolved
This pull request fixes 1 alert when merging bbb7cdc into c324083 - view on LGTM.com fixed alerts:
|
packages/amplify-graphql-auth-transformer/src/utils/definitions.ts
Outdated
Show resolved
Hide resolved
This pull request fixes 1 alert when merging 357f364 into 0b7a1a5 - view on LGTM.com fixed alerts:
|
Description of changes
This allows public subscriptions to be generated on models, and adds use of a new feature flag to deep merge directive defaults. For a description of the behavior, see aws-amplify/amplify-cli#10597
Issue #, if available
internally tracked
Description of how you validated changes
Unit testing and sample application
Checklist
yarn test
passesBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.