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.

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

Updated the resource type walkthrough #980

Merged

Conversation

mrog
Copy link
Contributor

@mrog mrog commented Mar 16, 2023

Issue #, if available:
979

Description of changes:
Updating the resource type walkthrough so it works with current versions of CloudFormation and dependencies.

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

[Setting up your environment for developing extensions](what-is-cloudformation-cli.md#resource-type-setup)
- Java 8
- For purposes of this walkthrough, it's assumed you have already set up the CloudFormation CLI and associated tooling for your Java development environment: [Setting up your environment for developing extensions](what-is-cloudformation-cli.md#resource-type-setup)
- This walkthrough uses an AMI that requires [a subscription](https://aws.amazon.com/marketplace/server/procurement?productId=7d426cb7-9522-4dd7-a56b-55dd8cc1c8d0) before you can use it with your account\.
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'm guessing this is the same AMI that was used in the original walkthrough. That old AMI ID isn't working anymore. The new AMI ID isn't usable without a free subscription.

@@ -24,10 +24,19 @@ This walkthrough uses the Community Edition of the [IntelliJ IDEA](https://www.j
Initializing new project
```

1. The `init` command launches a wizard that walks you through setting up the project, including specifying the resource name\. For this walkthrough, specify `Example::Testing::WordPress`\.
1. The `init` command launches a wizard that walks you through setting up the project, including specifying the resource name\. Start by specifying that you want to create a resource\.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The cfn init interface has changed a bit.

@@ -141,7 +164,6 @@ When you initiate the resource type project, an example resource type schema fil
},
"additionalProperties": false,
"primaryIdentifier": [
"/properties/PublicIp",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

PublicIp can't be part of the primary identifier. CloudFormation requires a public identifier in all responses, including the first one. The IP address isn't available until some time after the EC2 instance creation begins.


1. Open the project's `template.yml` file in the project's root directory\.

1. Find the `MemorySize` setting and change the value to `512`\.
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 was getting OutOfMemoryErrors with 256 MB.


1. In your IDE, open the `CreateHandler.java` file, located in the `src/main/java/com/example/testing/wordpress/CreateHandler.java` folder\.
We'll create a base class for the handlers. This simply provides a place to put code that would otherwise be duplicated by the various handlers\.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There were already two classes with the same code for getting EC2 instance info. I had to add a third class that also needed this data. Instead of duplicating the code again, I moved it to a shared location.

@@ -314,11 +359,14 @@ The CallbackContext is modeled as a POJO so you can define what information you
}

if (instanceStateSoFar == null) {
Instance instance = createEC2Instance(model);
model.setInstanceId(instance.getInstanceId());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The instance ID needs to be set here so the primary identifier can be included in the result.

@@ -407,7 +457,7 @@ The CallbackContext is modeled as a POJO so you can define what information you
.withSubnetIds(subnetId);

final DescribeSubnetsResult describeSubnetsResult =
clientProxy.injectCredentialsAndInvoke(describeSubnetsRequest, ec2Client::describeSubnets);
clientProxy.injectCredentialsAndInvoke(describeSubnetsRequest, (DescribeSubnetsRequest r) -> ec2Client.describeSubnets(r));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolving an ambiguous method name

@@ -438,20 +488,6 @@ The CallbackContext is modeled as a POJO so you can define what information you
.withTags(new Tag().withKey(SITE_NAME_TAG_KEY).withValue(siteName));
}

private Instance updatedInstanceProgress(String instanceId) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved to base class

doReturn(new RunInstancesResult().withReservation(new Reservation().withInstances(instance))).when(proxy).injectCredentialsAndInvoke(any(RunInstancesRequest.class), any());
doReturn(new CreateSecurityGroupResult().withGroupId("sg-1234")).when(proxy).injectCredentialsAndInvoke(any(CreateSecurityGroupRequest.class), any());
doReturn(new AuthorizeSecurityGroupIngressResult()).when(proxy).injectCredentialsAndInvoke(any(AuthorizeSecurityGroupIngressRequest.class), any());
doReturn(new DescribeSubnetsResult().withSubnets(new Subnet().withVpcId("vpc-1234"))).when(proxy).injectCredentialsAndInvoke(any(DescribeSubnetsRequest.class), ArgumentMatchers.<Function<DescribeSubnetsRequest, DescribeSubnetsResult>>any());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixing more ambiguous method names

@@ -775,7 +785,6 @@ Again, we'll implement the delete handler as a state machine\.
} else if (callbackContext.getInstance().getState().getName().equals(DELETED_INSTANCE_STATE)) {
callbackContext.getInstanceSecurityGroups().forEach(this::deleteSecurityGroup);
return ProgressEvent.<ResourceModel, CallbackContext>builder()
.resourceModel(model)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The delete handler must not return a model after a successful deletion.

@@ -807,20 +816,6 @@ Again, we'll implement the delete handler as a state machine\.
.orElse(new Instance());
}

private Instance currentInstanceState(String instanceId) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved to base class

@@ -890,7 +870,7 @@ We'll also need to update the unit test for the delete handler\.
@Test
public void testSuccessState() {
final DeleteSecurityGroupResult deleteSecurityGroupResult = new DeleteSecurityGroupResult();
doReturn(deleteSecurityGroupResult).when(proxy).injectCredentialsAndInvoke(any(DeleteSecurityGroupRequest.class), any());
doReturn(deleteSecurityGroupResult).when(proxy).injectCredentialsAndInvoke(any(DeleteSecurityGroupRequest.class), ArgumentMatchers.<Function<DeleteSecurityGroupRequest, DeleteSecurityGroupResult>>any());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolving an ambiguous method name

@@ -1085,6 +1065,190 @@ We'll also need to update the unit test for the delete handler\.
}
```

#### Code the Read Handler<a name="resource-type-walkthrough-implement-read-handler-code"></a>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

CloudFormation requires a read handler


Instance currentInstanceState = retrieveCurrentInstanceState(proxy, ec2Client, model.getInstanceId());

if (currentInstanceState.getInstanceId() == null || DELETED_INSTANCE_STATE.equals(currentInstanceState.getState().getName())) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The read handler must return a Not Found error if the instance has been deleted.

@@ -1200,7 +1364,7 @@ Occasionally these tests will fail with a retry\-able error\. In such a case, ru
},
"architecture": "x86_64",
"ebsOptimized": false,
"imageId": "ami-04fb0368671b6f138",
"imageId": "ami-0039c114b5e564742",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

New AMI

@@ -1458,3 +1622,56 @@ As a final test of the resource type delete handler, you can delete the `wordpre
aws cloudformation delete-stack --region us-west-2 \
--stack-name wordpress
```

## Clean up ##
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 like having clean up instructions.

@@ -1434,6 +1598,24 @@ Use [DescribeTypeRegistration](https://docs.aws.amazon.com/AWSCloudFormation/lat
"Name": "MyWebsite"
}
}
},
Copy link
Contributor Author

@mrog mrog Mar 16, 2023

Choose a reason for hiding this comment

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

I thought it would be nice to see some outputs here, since they're included in the model.

@ericzbeard ericzbeard requested review from mrinaudo-aws and mmaeng May 1, 2023 16:27
@ericzbeard ericzbeard added the documentation I'll give you a guess what this is about. label May 1, 2023
Copy link
Contributor

@mmaeng mmaeng left a comment

Choose a reason for hiding this comment

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

LGTM. Will look into the Github Actions, they should have completed by now.

@mmaeng
Copy link
Contributor

mmaeng commented May 1, 2023

@mrog If you could update your fork with the recent merges into the master branch? It looks like GitHub Actions got hung up for some reason on those 6 tests. Just want to kick start the workflow again.

@mrog
Copy link
Contributor Author

mrog commented May 2, 2023

@mmaeng I synced my fork. It should be okay to merge once the GitHub Actions are done. Thanks!

Copy link
Contributor

@mrinaudo-aws mrinaudo-aws left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution, @mrog! Please review/add the suggestion I made on the usage of Java 11, that is also supported, and that includes steps to follow if the user chooses to use Java 11 instead of Java 8. I was able to run contract tests with your changes as follows:

  • Contract tests using Java 8: 5 passed, 3 skipped, 16 deselected in 860.09s (0:14:20)
  • Contract tests using Java 11 (per the suggestion I added in the PR): 5 passed, 3 skipped, 16 deselected in 835.56s (0:13:55)

In addition to the above, I also tested the following: I submitted, to the private registry in the account I use and for the us-west-2 region, the resource type with your changes, that I compiled with Java 11 (see my suggestion in the PR), and was able to run it.

doc_source/resource-type-walkthrough.md Outdated Show resolved Hide resolved
doc_source/resource-type-walkthrough.md Outdated Show resolved Hide resolved
@mrinaudo-aws mrinaudo-aws self-requested a review May 3, 2023 17:15
Copy link
Contributor

@mrinaudo-aws mrinaudo-aws left a comment

Choose a reason for hiding this comment

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

LGTM

@mmaeng
Copy link
Contributor

mmaeng commented May 3, 2023

Looks like there are a few linting errors in the pre-commit. If you install pre-commit and run through the checks it should go through clean then. We are thinking we should be moving away from the pre-commit and just do it as part of the pipeline...

The install of pre-commit is defined in the Github actions file here (I would suggest a virtual env)
https://github.com/aws-cloudformation/cloudformation-cli/blob/master/.github/workflows/pr-ci.yaml

Assuming you are running on a Mac but from the cloudformation-cli root directory -
brew install autoconf automake libtool jq
pip install pyjq
pip install --upgrade 'attrs==19.2.0' wheel -r requirements.txt
pip install .
pre-commit run --all-files

Will have to run pre-commit a couple of times until it returns successful as it fixes linting issues. We have plans to rework pre-commit but it hasn't come to fruition yet.

Hope you don't mind but I pushed to your branch with the linting update.

@mmaeng mmaeng merged commit 6d42100 into aws-cloudformation:master May 4, 2023
@mmaeng
Copy link
Contributor

mmaeng commented May 9, 2023

Closes #979

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation I'll give you a guess what this is about.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants