Skip to content

Make certain request fields optional to unblock contract testing. #120

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

Merged
merged 4 commits into from
Sep 11, 2020

Conversation

johnttompkins
Copy link
Contributor

@johnttompkins johnttompkins commented Sep 8, 2020

Issue #, if available: #112 #109

Description of changes: This change makes some request fields optional/removes certain fields so that contract testing via cfn test can complete successfully without library errors. Below are a list of the changes to the request:

  • Made resourceType, resourceTypeVersion, and stackId optional. Making resourceType optional led to some issues with metrics publishing, so I had to refactor the MetricPublisher and MetricsPublisherProxy. This led to the layout being much more in line with the java plugin, where the proxy has no required arugments for construction and simply loops through available publishers when publishing. Any arguments pertinent to metric publishing are now passed into the actual publisher class, like resource type name and provider session. The changes to metrics publishing also conditionally create the publisher and logging when there are provider logging credentials and a log group name.

  • requestContext is removed as this is a relic of the original protocol and no longer relevant.

The first commit contains the bulk of these changes. The second commit just changes MetricPublisher->MetricsPublisher so the spelling is in line with the java plugin and the proxy :)

Testing done:

Created a dummy resource with the latest cloudformation-cli and ran cfn-test on a resource with this new support lib. Tests did not fail in the handler support lib like previously.

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

@johnttompkins
Copy link
Contributor Author

Testing this in a stack I got some unexpected errors. Going to investigate further before attempting to merge.

Copy link
Contributor

@ammokhov ammokhov left a comment

Choose a reason for hiding this comment

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

still investigating

@ammokhov ammokhov merged commit 36b217b into aws-cloudformation:master Sep 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Plugin using wrong cloudformation-cli-python-lib Test request throws exception with cloudformation-cli 0.1.6. Fails to parse new fields.
4 participants