Add new e2e test case for creating GSI's with already existing attributes #88
Conversation
ack-prow
bot
added
the
needs-ok-to-test
|
Hi @joaquinservetto. Thanks for your PR. I'm waiting for a aws-controllers-k8s member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
ack-prow
bot
added
ok-to-test
a-hilaly
changed the title
| gsi = { | ||
| "indexName": "total-bill", | ||
| "keySchema": [ | ||
| { | ||
| "attributeName": "Total", | ||
| "keyType": "HASH", | ||
| }, | ||
| { | ||
| "attributeName": "Bill", | ||
| "keyType": "RANGE", | ||
| } | ||
| ], | ||
| "projection": { | ||
| "projectionType": "ALL", | ||
| } | ||
| } |
There was a problem hiding this comment.
the controller is not able to patch this gsi and logs the error ValidationException: One or more parameter values were invalid: Both ReadCapacityUnits and WriteCapacityUnits must be specified for index: total-bill
There was a problem hiding this comment.
Weird, since It's n on-demand table It should not require the Read and Write capacity to be set, It's the bug that was solved by Julian here. But let me check.
There was a problem hiding this comment.
Can you review again? @a-hilaly
There was a problem hiding this comment.
Looks good to me and tests are passing, but still wondering why do we need to use a new table here? /cc @Julian-Chu can you please take a look?
There was a problem hiding this comment.
@a-hilaly do you mean table_basic_gsi_same_attr is not needed or combine the new test case into existing test case?
There was a problem hiding this comment.
Thank you guys, I'm not an expert either, should I modify the scope then?
There was a problem hiding this comment.
Maybe class or just function as the scope make more sense here.
There was a problem hiding this comment.
withscope="class", pytest will cache the fixture for entire class, so if we want table_basic to return different table name for different methods in a class, like @joaquinservetto mentioned, I think function is more suitable.
There was a problem hiding this comment.
Could you take a look again? @Julian-Chu @a-hilaly
There was a problem hiding this comment.
LGTM, my original idea is to reuse table_basic with function scope. it can be refactored later.
|
/test all |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: a-hilaly, joaquinservetto, Julian-Chu The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Issue aws-controllers-k8s/community#1920
Description of changes:
Add e2e test for table GSIs