Skip to content

Conversation

@ammokhov
Copy link
Contributor

@ammokhov ammokhov commented May 15, 2020

Issue #, if available: #265

Description of changes:

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

@ammokhov ammokhov added bug Something isn't working code quality codegen It's like magic! labels May 15, 2020
@ammokhov ammokhov linked an issue May 15, 2020 that may be closed by this pull request
@ammokhov ammokhov self-assigned this May 15, 2020
@ammokhov ammokhov requested review from PatMyron and miparnisari May 15, 2020 08:09
// TODO: uncomment when ready to implement
// if (exception instanceof CfnNotFoundException)
// return ProgressEvent.progress(model, context);
// throw exception;
Copy link
Contributor

Choose a reason for hiding this comment

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

I tested throwing exceptions here, and they are not bubbled all the way up. The handler ends up returning a status code of FAILED but no exception is thrown.

Copy link
Contributor

Choose a reason for hiding this comment

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

The intent is the Handler should return FAILED with a sane/consistent exception message that ends up in Stack Trace. I think there's a separate issue that needs to be fixed for that as it's not working reliably.

Copy link
Contributor

@miparnisari miparnisari May 15, 2020

Choose a reason for hiding this comment

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

@rjlohan So which of these asserts should I use?

assertThatThrownBy(() -> handler.handleRequest(proxy, request, new CallbackContext(), proxyClient, logger))
 .isInstanceOf(CfnAlreadyExistsException.class);

or

response = handler.handleRequest(proxy, request, new CallbackContext(), proxyClient, logger);
assertThat(response).isNotNull();
assertThat(response.getStatus()).isEqualTo(OperationStatus.FAILED);
assertThat(response.getErrorCode()).isEqualTo(HandlerErrorCode.AlreadyExists);

I've seen plenty of examples of the former but according to your suggestion we should use the latter?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working code quality codegen It's like magic!

Projects

None yet

Development

Successfully merging this pull request may close these issues.

codegen - create/delete handler pre execution check

5 participants