-
Notifications
You must be signed in to change notification settings - Fork 8
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
chore: Add calling AWS SDK from local test model #653
base: Golang/dev
Are you sure you want to change the base?
Conversation
var ddbClient :- expect Dynamodb.DynamoDBClient(); | ||
var resFailure := client.CallDDBScan(SimpleCallingAWSSDKFromLocalService.Types.CallDDBScanInput(ddbClient := ddbClient, tableArn := TABLE_ARN_FAILURE_CASE)); | ||
|
||
expect resFailure.Failure?; |
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.
We can also check for Opaque error over here but first we need to check if its DynamoDB or KMS error for Dafny verification to pass and for specifically Opaque error its little fragile in Go codegen. So, it doesn't have it but lmk if you think otherwise.
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.
Let's add a TODO to verify the error that get's thrown? I guess this is blocked on our codegen error conversion fix?
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.
I changed my mind and added a check for opaque error.
So, this is how codegen would work in next PR (also in comment)
- Go delegates the error first to KMS
- If KMS is opaque, it is going to delegate to DDB. And return a DDB error (opaque or non opaque)
- If KMS is not opaque, it returns ComAmazonawsKms error.
So, there is no KMS opaque error.
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.
Also, DDB give opaque error when trying to access table not in the account for user with access to limited number of table because they can't say resource not found (because user does not have access to all table) and they give AccessDeniedException which is not DDB exception but an IAM exception.
var ddbClient :- expect Dynamodb.DynamoDBClient(); | ||
var resFailure := client.CallDDBScan(SimpleCallingAWSSDKFromLocalService.Types.CallDDBScanInput(ddbClient := ddbClient, tableArn := TABLE_ARN_FAILURE_CASE)); | ||
|
||
expect resFailure.Failure?; |
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.
Let's add a TODO to verify the error that get's thrown? I guess this is blocked on our codegen error conversion fix?
|
||
expect resFailure_NonExistent.Failure?; | ||
expect resFailure_NonExistent.error.ComAmazonawsKms?; | ||
expect resFailure_NonExistent.error.ComAmazonawsKms.NotFoundException?; |
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.
Ohh I see it's here. Why wasn't this possible in DDB case?
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.
I think my previous comment would address this comment. lmk if it does not.
const TABLE_ARN_FAILURE_CASE := "arn:aws:dynamodb:us-west-2:370957321024:table/TestTableFailure" | ||
const NONEXISTENT_KEY_ID := "arn:aws:kms:us-west-2:658956600833:key/b3537ef1-d8dc-4780-9f5a-55776cbb2f7g" | ||
// The string "asdf" as bytes | ||
const PLAIN_TEXT: Kms.Types.PlaintextType := [ 97, 115, 100, 102 ] |
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.
I think we aren't using these, remove them if we are not.
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.
Yeah. I missed removing this while doing clean. My bad.
|
||
static { | ||
DISABLED_TESTS.add("AggregateReferences"); | ||
DISABLED_TESTS.add("CallingAWSSDKFromLocalService"); |
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.
Why do we have it disabled for Go?
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.
Just to make CI pass. I will make a PR next to fix codegen next and remove this disabled test
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.
Also, I was initially think to make it as a single PR but thought that single PR could be too long. So, I divided that PR into 2 parts. This is 1st part and it is already 16 files long.
This PR LGTM. Let's enable the tests before merge. |
Issue #, if available:
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.