Skip to content

Conversation

@ammokhov
Copy link
Contributor

@ammokhov ammokhov commented Oct 1, 2020

Issue #, if available:

Description of changes:

adding support for snapshot metadata

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

@ammokhov ammokhov self-assigned this Oct 1, 2020
@ammokhov ammokhov added codegen It's like magic! enhancement New feature or request labels Oct 1, 2020
Copy link
Contributor

@aygold92 aygold92 left a comment

Choose a reason for hiding this comment

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

is there a way to test this end to end before we submit it?

.logicalResourceIdentifier(request.getRequestData().getLogicalResourceId())
.nextToken(request.getNextToken())
.region(request.getRegion())
.snapshotRequested(request.getSnapshotRequested())
Copy link

Choose a reason for hiding this comment

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

We should not add change in the generated code here. This will cause build break if only upgrade the PyPi java plugin.

The better place like this https://github.com/aws-cloudformation/cloudformation-cli-java-plugin/blob/master/src/main/java/software/amazon/cloudformation/LambdaWrapper.java#L275

We won't even need tool release.

private String region;
private String resourceType;
private String resourceTypeVersion;
private RequestData<ResourceT> requestData;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: should this be in request data instead? seems relatedto the actual request and not cloudformation metadata related info

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point, but TBH I don't really understand the distinction here anyways. Like, why are providerCredentials, providerLogGroupName, and logicalResourceId part of requestData while callbackContext, accountId, bearerToken, and action are not?

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

Labels

codegen It's like magic! enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants