feat: dotnet examples #482
Conversation
There was a problem hiding this comment.
I assume that these are used somewhere.
Should we put this list in Dafny so that every runtime has access to the same strings?
There was a problem hiding this comment.
It's the same list used in the Java example.
I presume that once I've moved over all the examples, that all these will be used.
Putting them in a central place seems reasonable. I'm not sure Dafny is the right choice, but maybe I just don't see whatever you're envisioning.
| var kmsKeyId = TestUtils.TEST_KMS_KEY_ID; | ||
| var ddbTableName = TestUtils.TEST_DDB_TABLE_NAME; |
There was a problem hiding this comment.
Users should be able to pass in their own resource ARNs via a CLI args.
Two questions:
- Could you have PutItemGetItem take in these arguments?
- Could you add a main method to the bottom of this file that takes in command-line arguments and passes them to PutItemGetItem?
| @@ -0,0 +1,19 @@ | |||
| | |||
There was a problem hiding this comment.
Users should be able to execute examples independently of one another.
I think you should remove this file in favor of per-example Main methods.
Also for parsing args and passing them into the example method.
| using System; | ||
| public class TestUtils | ||
| { | ||
| public static readonly string TEST_KEYSTORE_NAME = "KeyStoreDdbTable"; |
There was a problem hiding this comment.
Can this go in a runtimes/net/tests or similar directory?
Then add a Tests.csproj file that executes the tests similar to https://github.com/aws/aws-encryption-sdk-dafny/blob/mainline/aws-encryption-sdk-net/Test/AWSEncryptionSDKTests.csproj
| @@ -32,7 +32,7 @@ module PutItemTransform { | |||
| //= type=implication | |||
| //# The PutItem request MUST NOT refer to any legacy parameters, | |||
| //# specifically Expected and ConditionalOperator MUST NOT be set. | |||
| && input.sdkInput.Expected.None? && input.sdkInput.ConditionalOperator.None? | |||
| && NoMap(input.sdkInput.Expected) && input.sdkInput.ConditionalOperator.None? | |||
There was a problem hiding this comment.
Wow. If this is all that is needed to handle empty, we should change Java.
(But treat DDB's AttributeValue specially)
There was a problem hiding this comment.
For an optional map or list, the dotnet AWS SDK can't distinguish empty from None (since it hides the IsSet method).
This should be safe for java, since the smithy says "Optional non-empty".
I don't understand what you mean by "we should change java" -- as far as I can tell, Java is right and dotnet is wrong.
| <ImplicitUsings>enable</ImplicitUsings> | ||
| <Nullable>disable</Nullable> | ||
| <LangVersion>10</LangVersion> | ||
| <TargetFramework>net6.0</TargetFramework> |
There was a problem hiding this comment.
This would be a breaking change. I don't think we can do this.
There was a problem hiding this comment.
At some point recently, it told me MPL didn't support net48. Maybe changing ProjectReference to PackageReference will fix that.
| <ItemGroup> | ||
| <PackageReference Include="AWSSDK.DynamoDBv2" Version="3.7.300.2"/> | ||
| <PackageReference Include="AWSSDK.Core" Version="3.7.300.2"/> | ||
| <ProjectReference Include="../../../submodules/MaterialProviders/AwsCryptographicMaterialProviders/runtimes/net/MPL.csproj" /> |
There was a problem hiding this comment.
I don't think that this will work when the code is published.
There was a problem hiding this comment.
Good point. I'll switch it back.
There was a problem hiding this comment.
@josecorella I think that the sub-module MPL here is only 6.0,
but I think that upstream is 6 and 4.8?
There was a problem hiding this comment.
yes, the mpl submodule commit on the dotnet-example is very out of date
https://github.com/aws/aws-cryptographic-material-providers-library-dafny/blob/13e0ac3c3c5eea83494706e4a96f40126d8f38a8/AwsCryptographicMaterialProviders/runtimes/net/MPL.csproj#L8
There was a problem hiding this comment.
@ajewellamz we should update the mpl submodule in this pr to point to the latest mpl changes
| "BS": [] | ||
| "BS": [""] |
There was a problem hiding this comment.
Just wondering why change this?
There was a problem hiding this comment.
Because empty sets are illegal. I don't know how is passed in Java.
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.