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.

Sign up for GitHub

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

Jump to bottom

feat(graphql-key-transformer): change default to add GSIs when using @key #5648

Merged
merged 3 commits into from
Jan 28, 2021
Merged

feat(graphql-key-transformer): change default to add GSIs when using @key #5648

merged 3 commits into from
Jan 28, 2021

Conversation

SwaySway
Copy link
Contributor

@SwaySway SwaySway commented Oct 21, 2020 •
edited
Loading

Description of changes:

  • changed the default to adding GSI when creating a secondary index
  • Added sanity check when removing an LSI from an already existing table
  • Will maintain previous behavior via a feature flag
{
  "features":
  {
    "graphqltransformer":
    {
      "secondaryKeyAsGSI": true
    }
  }
}

Issue #, if available:

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

@SwaySway SwaySway mentioned this pull request Oct 21, 2020
Closed
@SwaySway SwaySway linked an issue Oct 21, 2020 that may be closed by this pull request
API (GraphQL with DynamoDB): Creating GSI reusing table's Partition Key #2702
Closed
@codecov
Copy link

codecov bot commented Oct 21, 2020 •
edited
Loading

Codecov Report

Merging #5648 (af73cd5) into master (39dfd27) will decrease coverage by 0.02%.
The diff coverage is 23.07%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5648      +/-   ##
==========================================
- Coverage   57.28%   57.26%   -0.03%     
==========================================
  Files         479      479              
  Lines       21695    21707      +12     
  Branches     4096     4098       +2     
==========================================
+ Hits        12429    12431       +2     
- Misses       8405     8415      +10     
  Partials      861      861
Impacted Files Coverage Δ
...amplify-cli-core/src/feature-flags/featureFlags.ts 83.10% <ø> (ø)
.../graphql-transformer-core/src/util/amplifyUtils.ts 5.82% <ø> (ø)
.../graphql-transformer-core/src/util/sanity-check.ts 14.38% <9.09%> (-0.46%) ⬇️
...ages/graphql-key-transformer/src/KeyTransformer.ts 91.78% <100.00%> (+0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 39dfd27...af73cd5. Read the comment docs.

edwardfoyle
edwardfoyle reviewed Nov 16, 2020
View reviewed changes
packages/graphql-transformer-core/src/util/sanity-check.ts Outdated Show resolved Hide resolved
packages/graphql-transformer-core/src/util/sanity-check.ts Outdated Show resolved Hide resolved
packages/graphql-transformer-core/src/util/sanity-check.ts Outdated Show resolved Hide resolved
packages/amplify-e2e-core/src/utils/projectMeta.ts Outdated
Comment on lines 4 to 12
function getFeatureFlagConfig(projRoot: string) {
const featureFlagPath = path.join(projRoot, 'amplify', 'cli.json');
return JSON.parse(fs.readFileSync(featureFlagPath, 'utf8'));
}

function updateFeatureFlagConfig(projRoot: string, config: any) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be cleaner to collapse both of these functions into setFeatureFlag(projRoot: string, feature: string, key: string, value: any) and it handles reading, updating and writing the cli.json file internally

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we should have a set and get for this. In event we need to read from the config to determine what to change it to as opposed to overriding the config.

packages/amplify-cli-core/src/feature-flags/featureFlags.ts Outdated Show resolved Hide resolved
packages/graphql-key-transformer/src/KeyTransformer.ts Outdated Show resolved Hide resolved
@edwardfoyle
Copy link
Contributor

Also, can you link a docs PR that adds this to the FF section: https://docs.amplify.aws/cli/reference/feature-flags#graphQLTransformer

@SwaySway SwaySway mentioned this pull request Jan 4, 2021
Closed
@SwaySway SwaySway requested a review from a team as a code owner January 20, 2021 22:37
@lgtm-com
Copy link

lgtm-com bot commented Jan 20, 2021

This pull request introduces 3 alerts when merging 1b2de33 into 4ce4138 - view on LGTM.com

new alerts:

  • 3 for Syntax error

@SwaySway SwaySway force-pushed the keyGSIDefault branch 2 times, most recently from dc00a7d to b7e6d0d Compare January 21, 2021 03:06
@SwaySway SwaySway requested a review from edwardfoyle January 21, 2021 03:06
@SwaySway
Copy link
Contributor Author

SwaySway commented Jan 21, 2021 •
edited by jhockett
Loading

Docs PR
aws-amplify/docs#2873

edwardfoyle
edwardfoyle reviewed Jan 26, 2021
View reviewed changes
Copy link
Contributor

@edwardfoyle edwardfoyle left a comment

Choose a reason for hiding this comment

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

just a few small comments. Functionality LGTM

packages/amplify-e2e-core/src/utils/projectMeta.ts Outdated Show resolved Hide resolved
packages/graphql-transformers-e2e-tests/src/__tests__/KeyTransformerLocal.e2e.test.ts Outdated Show resolved Hide resolved
packages/graphql-transformers-e2e-tests/src/__tests__/KeyTransformerLocal.e2e.test.ts Outdated Show resolved Hide resolved
SwaySway added 2 commits January 27, 2021 18:46
feat(graphql-key-transformer): change @key to create gsis
ada94fc 
this change will ensure that @key will create a GSI instead of an LSI when using a field that is
also a partition key

re aws-amplify#2702
test: add migration test to check for removing LSIs
69b1e48
@SwaySway SwaySway requested a review from edwardfoyle January 28, 2021 20:32
@jhockett jhockett merged commit 4287c63 into aws-amplify:master Jan 28, 2021
@edwardfoyle edwardfoyle mentioned this pull request Jan 30, 2021
Closed
@edwardfoyle edwardfoyle mentioned this pull request Feb 22, 2021
Closed
@github-actions
Copy link

This pull request has been automatically locked since there hasn't been any recent activity after it was closed. Please open a new issue for related bugs.

Looking for a help forum? We recommend joining the Amplify Community Discord server *-help channels for those types of questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@edwardfoyle edwardfoyle edwardfoyle approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

API (GraphQL with DynamoDB): Creating GSI reusing table's Partition Key
3 participants
@SwaySway @edwardfoyle @jhockett