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

added some pre-actionable checks for update and read handler to align with handler contract #294

Merged
merged 2 commits into from
Aug 4, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ protected ProgressEvent<ResourceModel, CallbackContext> handleRequest(
// STEP 1.3 [TODO: handle exception]
.handleError((awsRequest, exception, client, model, context) -> {
// TODO: uncomment when ready to implement
// if (exception instanceof CfnNotFoundException)
// if (exception instanceof ResourceNotFoundException)
// return ProgressEvent.success(model, context);
// throw exception;
return ProgressEvent.progress(model, context);
Expand Down Expand Up @@ -105,6 +105,10 @@ protected ProgressEvent<ResourceModel, CallbackContext> handleRequest(
logger.log(String.format("%s [%s] deletion has stabilized: %s", ResourceModel.TYPE_NAME, model.getPrimaryIdentifier(), stabilized));
return stabilized;
})
.success());
.progress()
)

// STEP 3 [TODO: return the successful progress event without resource model]
.then(progress -> ProgressEvent.defaultSuccessHandler(null));
anshikg marked this conversation as resolved.
Show resolved Hide resolved
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,11 @@ public void handleRequest_SimpleSuccess() {
assertThat(response).isNotNull();
assertThat(response.getStatus()).isEqualTo(OperationStatus.SUCCESS);
assertThat(response.getCallbackDelaySeconds()).isEqualTo(0);
{% if operation == "Delete" %}
assertThat(response.getResourceModel()).isNull();
{% else %}
anshikg marked this conversation as resolved.
Show resolved Hide resolved
assertThat(response.getResourceModel()).isEqualTo(request.getDesiredResourceState());
{% endif %}
assertThat(response.getResourceModels()).isNull();
assertThat(response.getMessage()).isNull();
assertThat(response.getErrorCode()).isNull();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@ protected ProgressEvent<ResourceModel, CallbackContext> handleRequest(
try {

// TODO: add custom read resource logic
// If describe request does not return ResourceNotFoundException, you must throw ResourceNotFoundException based on
// awsResponse values

} catch (final AwsServiceException e) { // ResourceNotFoundException
/*
Expand Down
46 changes: 37 additions & 9 deletions python/rpdk/java/templates/init/guided_aws/StubUpdateHandler.java
Original file line number Diff line number Diff line change
Expand Up @@ -29,17 +29,45 @@ protected ProgressEvent<ResourceModel, CallbackContext> handleRequest(
// https://github.com/aws-cloudformation/cloudformation-cli-java-plugin/blob/master/src/main/java/software/amazon/cloudformation/proxy/CallChain.java

return ProgressEvent.progress(request.getDesiredResourceState(), callbackContext)
// STEP 1 [first update/stabilize progress chain - required for resource update]

// STEP 1 [check if resource already exists]
// for more information -> https://docs.aws.amazon.com/cloudformation-cli/latest/userguide/resource-type-test-contract.html
// if target API does not support 'ResourceNotFoundException' then following check is required
Comment on lines +32 to +35
Copy link
Contributor

Choose a reason for hiding this comment

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

it seems we are checking for not found at multiple places. May be consolidate this to be in on place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we might be having same callGraph value cached in case you cross handler interaction. also I think it's already quite wordy if we refer in multiple places it will be confusing

.then(progress ->
// STEP 1.0 [initialize a proxy context]
// If your service API does not return ResourceNotFoundException on update requests against some identifier (e.g; resource Name)
// and instead returns a 200 even though a resource does not exist, you must first check if the resource exists here
// NOTE: If your service API throws 'ResourceNotFoundException' for update requests this method is not necessary
Copy link
Contributor

Choose a reason for hiding this comment

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

Even if the service API throws ResourceNotFoundException, the contract tests as currently implemented expect ResourceNotFoundException to be thrown even when calling the handlers with obviously invalid inputs which likely will hit model validation before checking if the resource exists or not, even for services that do throw ResourceNotFoundException

also discussed in aws-cloudformation/cloudformation-cli#527

proxy.initiate("{{ call_graph }}::{{ operation }}::PreUpdateCheck", proxyClient, progress.getResourceModel(), progress.getCallbackContext())

// STEP 1.1 [initialize a proxy context]
.translateToServiceRequest(Translator::translateToReadRequest)

// STEP 1.2 [TODO: make an api call]
.makeServiceCall((awsRequest, client) -> {
AwsResponse awsResponse = null;

// TODO: add custom read resource logic
// If describe request does not return ResourceNotFoundException, you must throw ResourceNotFoundException based on
// awsResponse values

logger.log(String.format("%s has successfully been read.", ResourceModel.TYPE_NAME));
return awsResponse;
})
.progress()
)

// STEP 2 [first update/stabilize progress chain - required for resource update]
.then(progress ->
// STEP 2.0 [initialize a proxy context]
// Implement client invocation of the update request through the proxyClient, which is already initialised with
// caller credentials, correct region and retry settings
proxy.initiate("{{ call_graph }}::{{ operation }}::first", proxyClient, progress.getResourceModel(), progress.getCallbackContext())

// STEP 1.1 [TODO: construct a body of a request]
// STEP 2.1 [TODO: construct a body of a request]
.translateToServiceRequest(Translator::translateToFirstUpdateRequest)

// STEP 1.2 [TODO: make an api call]
// STEP 2.2 [TODO: make an api call]
.makeServiceCall((awsRequest, client) -> {
AwsResponse awsResponse = null;
try {
Expand All @@ -60,7 +88,7 @@ protected ProgressEvent<ResourceModel, CallbackContext> handleRequest(
return awsResponse;
})

// STEP 1.3 [TODO: stabilize step is not necessarily required but typically involves describing the resource until it is in a certain status, though it can take many forms]
// STEP 2.3 [TODO: stabilize step is not necessarily required but typically involves describing the resource until it is in a certain status, though it can take many forms]
// stabilization step may or may not be needed after each API call
// for more information -> https://docs.aws.amazon.com/cloudformation-cli/latest/userguide/resource-type-test-contract.html
.stabilize((awsRequest, awsResponse, client, model, context) -> {
Expand All @@ -74,17 +102,17 @@ protected ProgressEvent<ResourceModel, CallbackContext> handleRequest(
.progress())

// If your resource is provisioned through multiple API calls, then the following pattern is required (and might take as many postUpdate callbacks as necessary)
// STEP 2 [second update/stabilize progress chain]
// STEP 3 [second update/stabilize progress chain]
.then(progress ->
// STEP 2.0 [initialize a proxy context]
// STEP 3.0 [initialize a proxy context]
// If your resource is provisioned through multiple API calls, you will need to apply each subsequent update
// step in a discrete call/stabilize chain to ensure the entire resource is provisioned as intended.
proxy.initiate("{{ call_graph }}::{{ operation }}::second", proxyClient, progress.getResourceModel(), progress.getCallbackContext())

// STEP 2.1 [TODO: construct a body of a request]
// STEP 3.1 [TODO: construct a body of a request]
.translateToServiceRequest(Translator::translateToSecondUpdateRequest)

// STEP 2.2 [TODO: make an api call]
// STEP 3.2 [TODO: make an api call]
.makeServiceCall((awsRequest, client) -> {
AwsResponse awsResponse = null;
try {
Expand All @@ -106,7 +134,7 @@ protected ProgressEvent<ResourceModel, CallbackContext> handleRequest(
})
.progress())

// STEP 3 [TODO: describe call/chain to return the resource model]
// STEP 4 [TODO: describe call/chain to return the resource model]
.then(progress -> new ReadHandler().handleRequest(proxy, request, callbackContext, proxyClient, logger));
}
}
1 change: 1 addition & 0 deletions python/rpdk/java/templates/init/guided_aws/Translator.java
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ static ResourceModel translateFromReadResponse(final AwsResponse awsResponse) {
// e.g. https://github.com/aws-cloudformation/aws-cloudformation-resource-providers-logs/blob/2077c92299aeb9a68ae8f4418b5e932b12a8b186/aws-logs-loggroup/src/main/java/com/aws/logs/loggroup/Translator.java#L58-L73
return ResourceModel.builder()
//.someProperty(response.property())
// IMPORTANT: exclude writeOnlyProperties
Copy link
Contributor

Choose a reason for hiding this comment

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

isn't this done by lambda wrapper?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes it should be. At least in this language plugin.

Copy link
Contributor

Choose a reason for hiding this comment

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

could you just remove this line about write only properties. This might cause some confusion.

.build();
}

Expand Down