Fixing some of the contract tests for Domain. #22

jonjara · 2020-08-17T21:54:44Z

Issue #, if available:

Description of changes:
Adding in some fixes that make the handlers to adhere to https://docs.aws.amazon.com/cloudformation-cli/latest/userguide/resource-type-test-contract.html.

contract_invalid_create still doesn't pass and I am going to reach out to CloudFormation to get guidance on this test

NOTES:

I am manually adding in arns to the ListHandlers but we will not do this when ListDomains populates the arn in it's sdk response. The reason it's needed is because the primary identifier needs to be populated after a list operation
Also removing domainOwner from some of the operations because I am assuming that for for update/read permissions policies and describe domain, we are always doing so in the account the CFN stack is in.

Testing

handler_create.py::contract_create_delete PASSED                         [  6%]
handler_create.py::contract_invalid_create PASSED                        [ 12%]
handler_create.py::contract_create_duplicate SKIPPED                     [ 18%]
handler_create.py::contract_create_read_success PASSED                   [ 25%]
handler_create.py::contract_create_list_success PASSED                   [ 31%]
handler_delete.py::contract_delete_read PASSED                           [ 37%]
handler_delete.py::contract_delete_list PASSED                           [ 43%]
handler_delete.py::contract_delete_update PASSED                         [ 50%]
handler_delete.py::contract_delete_delete PASSED                         [ 56%]
handler_delete.py::contract_delete_create SKIPPED                        [ 62%]
handler_misc.py::contract_check_asserts_work PASSED                      [ 68%]
handler_read.py::contract_read_without_create PASSED                     [ 75%]
handler_update.py::contract_update_read_success PASSED                   [ 81%]
handler_update.py::contract_update_list_success PASSED                   [ 87%]
handler_update_invalid.py::contract_update_create_only_property PASSED   [ 93%]
handler_update_invalid.py::contract_update_non_existent_resource PASSED  [100%]

When I run the tests that ERROR by themselves they pass

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

joeymarylander · 2020-08-17T22:35:56Z

aws-codeartifact-domain/src/main/java/software/amazon/codeartifact/domain/DeleteHandler.java

+                if (!doesDomainExist(request.getDesiredResourceState(), proxyClient, request)) {
+                    throw new CfnNotFoundException(request.getDesiredResourceState().getDomainName(), ResourceModel.TYPE_NAME);
+                }


This is interesting to me as I know that a bunch of other CFN resources will not throw an exception if we are trying to delete something that is already deleted.... Do you know if this would show up in the console? Or if this is magically handled by CFN to be OK

Just ran a test, internally the exception gets thrown. From the logs:

Resource of type 'domain-name' with identifier 'AWS::CodeArtifact::Domain' was not found. in a DELETE action on a AWS::CodeArtifact::Domain: software.amazon.cloudformation.exceptions.CfnNotFoundException: Resource of type 'poc-domain' with identifier 'AWS::CodeArtifact::Domain' was not found.

In the console, the stack ends up in DELETE_COMPLETE, so it seems to be handled

joeymarylander · 2020-08-17T22:37:51Z

aws-codeartifact-domain/src/main/java/software/amazon/codeartifact/domain/Translator.java

    return streamOfOrEmpty(awsResponse.domains())
        .map(domain -> ResourceModel.builder()
+            .arn(buildArn("us-west-2", domain.owner(), domain.name()))


why are we hardcoding us-west-2?

cfn test does not override regions and defaults to us-west-2, I hardcoded this and forgot to remove it :(

joeymarylander · 2020-08-17T22:38:34Z

aws-codeartifact-domain/src/main/java/software/amazon/codeartifact/domain/Translator.java

+    final String partition = "aws";
+    if (region.contains("us-gov-")) {
+      partition.concat("-us-gov");
+    }
+
+    if (region.contains("cn-")) {
+      partition.concat("-cn");
+    }


I get that this is temporary until we can get the ListDomains to return the Arns, but i am surprised at the very least that there isnt some magic "give me my partition" in the CFN toolkit here. I know if you create a Region object (from sddv2) you can get the partition.

joeymarylander · 2020-08-17T22:48:23Z

aws-codeartifact-domain/src/main/java/software/amazon/codeartifact/domain/Translator.java

-    String accountId = request.getAwsAccountId();
    return GetDomainPermissionsPolicyRequest.builder()
        .domain(model.getDomainName())
-        .domainOwner(accountId)
        .build();


kinda odd that we were first using the caller's accountId instead of just grabbing it from the model itself

and now we are just omitting it. I get that the caller should be the domain owner in this case. But if we decide that anyone can create an independent resource for a DomainPolicy, we might pass in the domain Owner to that method and that would then need to be translated accordingly

joeymarylander

My comments are just nits. If you want to include them in another PR thats fine. If you ignore them, ill probably forget and tell you to do them in another PR haha, so up to you how you want to handle them

joeymarylander · 2020-08-24T17:27:33Z

aws-codeartifact-domain/src/main/java/software/amazon/codeartifact/domain/BaseHandlerStd.java


 public abstract class BaseHandlerStd extends BaseHandler<CallbackContext> {
-
+  public static final ObjectMapper MAPPER = new ObjectMapper();


nit, this should be private, or if you're using this in subclasses, then protected

joeymarylander · 2020-08-24T17:29:25Z

aws-codeartifact-domain/src/main/java/software/amazon/codeartifact/domain/BaseHandlerStd.java

+        try {
+            proxyClient.injectCredentialsAndInvokeV2(
+                Translator.translateToReadRequest(model, request), proxyClient.client()::describeDomain);
+            return true;
+        } catch (ResourceNotFoundException e) {
+            return false;
+        }
    }


not sure if this little green squares mean you have weird whitespace or something. Maybe accidentally had a couple of tabs instead of spaces?

joeymarylander · 2020-08-24T17:31:22Z

aws-codeartifact-domain/src/main/java/software/amazon/codeartifact/domain/DeleteHandler.java

@@ -24,44 +23,43 @@
        final Logger logger) {

        this.logger = logger;
+        ResourceModel model = request.getDesiredResourceState();
+        // STEP 1.0 [initialize a proxy context]
+        ProgressEvent<ResourceModel, CallbackContext> test = proxy


nit, i'd call this something other than test.

oops, I was supposed to change this to something meaningful..

PatMyron · 2020-11-03T18:37:11Z

aws-codeartifact-domain/aws-codeartifact-domain.json

+      "type": "object",
      "minLength": 2,
      "maxLength": 5120


#60
minLength/maxLength is specifically for string types
issue to catch these automatically

Suggested change

"type": "object",

"minLength": 2,

"maxLength": 5120

"type": "object",

joeymarylander suggested changes Aug 17, 2020

View reviewed changes

jonjara force-pushed the domain-contract-tests branch 4 times, most recently from 82e2fcb to f20a56b Compare August 19, 2020 23:25

Addressing feedback and getting all the contract tests to pass.

54f130c

jonjara force-pushed the domain-contract-tests branch from f20a56b to 54f130c Compare August 20, 2020 05:45

sankalpbhandari approved these changes Aug 20, 2020

View reviewed changes

joeymarylander approved these changes Aug 24, 2020

View reviewed changes

Addressing some nits.

53ba117

jonjara force-pushed the domain-contract-tests branch from 4dc59b0 to 53ba117 Compare August 24, 2020 18:06

jonjara merged commit 78a850c into aws-cloudformation:main Aug 24, 2020

PatMyron reviewed Nov 3, 2020

View reviewed changes

PatMyron mentioned this pull request Nov 3, 2020

cfn validate fails for wrong min/max constraint aws-cloudformation/cloudformation-cli#414

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixing some of the contract tests for Domain. #22

Fixing some of the contract tests for Domain. #22

jonjara commented Aug 17, 2020 •

edited

Loading

joeymarylander Aug 17, 2020

jonjara Aug 18, 2020

joeymarylander Aug 17, 2020

jonjara Aug 18, 2020

joeymarylander Aug 17, 2020

joeymarylander Aug 17, 2020

joeymarylander left a comment

joeymarylander Aug 24, 2020

jonjara Aug 24, 2020

joeymarylander Aug 24, 2020

joeymarylander Aug 24, 2020

jonjara Aug 24, 2020

PatMyron Nov 3, 2020 •

edited

Loading


		public abstract class BaseHandlerStd extends BaseHandler<CallbackContext> {

		public static final ObjectMapper MAPPER = new ObjectMapper();

Fixing some of the contract tests for Domain. #22

Fixing some of the contract tests for Domain. #22

Conversation

jonjara commented Aug 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joeymarylander left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PatMyron Nov 3, 2020 • edited Loading

Choose a reason for hiding this comment

jonjara commented Aug 17, 2020 •

edited

Loading

PatMyron Nov 3, 2020 •

edited

Loading