aws-cloudformation / aws-cloudformation-resource-providers-cloudformation

The CloudFormation Resource Provider Package For AWS CloudFormation
https://aws.amazon.com/cloudformation/
Apache License 2.0
48 stars 35 forks source link

do not check if module default version exists #50

Closed tylersouthwick closed 3 years ago

tylersouthwick commented 3 years ago

Issue #, if available:

49

Description of changes: I had looked at checking if the default version is the same as the one that we're trying to set. But it seems that that the API doesn't throw an error in that case. The only issue I can see is that this could allow a different stack to manage the same default version resource.

ResourceDefaultVersion does not have this check.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

priyanka-amzn commented 3 years ago

Hello, This PR proposes to remove the AlreadyExists check that we put in place for consistency keeping expected behavior for all Registry resource-types in mind.

More details on the check:

priyanka-amzn commented 3 years ago

When a user uses a ModuleVersion and ModuleDefaultVersion in the same template after de-registering all the versions of this ModuleVersion earlier, the user can include only ModuleVersion in the template. ModuleDefaultVersion would not be required. Including it will return an error because it tries to set a version that is already the default version as the default version.

This is because if the CloudFormation Registry service gets a request to create a new ModuleVersion and there is no existing default version for it, it sets that version as the default version.

ModuleDefaultVersion is to be used when there are multiple versions and the customer wants to change the default version.

tylersouthwick commented 3 years ago

@priyanka-amzn please see the comment I added in #49

priyanka-amzn commented 3 years ago

Thanks for your patience Tyler. We've discussed this with managers and decided that removing the AlreadyExists exception in ModuleDefaultVersion would be better for customers and this would also make it consistent with ResourceDefaultVersion's behavior.

@kristian-d and I will review this PR. Thanks again for your contribution.

priyanka-amzn commented 3 years ago

@kristian-d please review.

kfdarlington commented 3 years ago

Looks good! I just left one minor comment on a unit test.

tylersouthwick commented 3 years ago

@kristian-d I addressed the comment.