aws-cloudformation / cloudformation-resource-schema

The CloudFormation Resource Schema defines the shape and semantic for resources provisioned by CloudFormation. It is used by provider developers using the CloudFormation RPDK.
Apache License 2.0
90 stars 38 forks source link

Add compare method for create only properties #120

Closed anshikg closed 3 years ago

anshikg commented 3 years ago

Issue #, if available:

Description of changes: Right now, if the create only property passed to the update handler is different from the created model it is the handler's responsibility to throw a not update-able exception. This works for now. But in my opinion we should have this check in the language plugins and support this functionality on the framework vs asking the handlers to throw the right error code. This CR is creating a method to compare the previous model's create only property with the updated model's create only properties and if they are different the appropriate exception is going to be thrown by the language plugins. (Validation in language plugins will be a separate CR)

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

PatMyron commented 3 years ago

1) @aygold92 wondering if platform can generally handle this instead of handling this in each plugin / in each handler

2) @ammokhov this also needs to consider conditionallyCreateOnlyProperties once we have those modelable

aygold92 commented 3 years ago

can you be more specific in the PR description (and inline too, probably) what this is "comparing"/for what purpose?

anshikg commented 3 years ago

What's this to be used for?

Updated the details section with the usage

anshikg commented 3 years ago
1. @aygold92 wondering if platform can generally handle this instead of handling this in each plugin / in each handler

2. @ammokhov this also needs to consider [`conditionallyCreateOnlyProperties`](https://github.com/aws-cloudformation/aws-cloudformation-resource-schema/issues/30) once we have those modelable

good point. i feel like if we add it to java plugin then it becomes a hard requirement on any other future plugins, which is not obvious. If we go that route then we should have a strict requirements on what plugin must satisfy, which we currently dont have.

I also think this is a bit redundant and probably not necessary

This is already a hard requirement as this is asserted by Contract test: https://github.com/aws-cloudformation/cloudformation-cli/blob/master/src/rpdk/core/contract/suite/handler_update_invalid.py#L20 All this CR is doing is pushing the validation upstream in the framework vs having the check in the handlers. Also, this is also one of the handler contracts: Any createOnlyProperties specified in update handler input MUST NOT be different from their previous state.

anshikg commented 3 years ago

additionally, I dont think this is the best place to put immutability analyzer.

this is invoked twice in java plugin: https://github.com/aws-cloudformation/cloudformation-cli-java-plugin/blob/master/src/main/java/software/amazon/cloudformation/AbstractWrapper.java#L201 which is to hide deserialization mismatch

and the other one: https://github.com/aws-cloudformation/cloudformation-cli-java-plugin/blob/master/src/main/java/software/amazon/cloudformation/AbstractWrapper.java#L291 when we already published metrics and started request transformation

Could you help me understand why I dont think this is the best place to put immutability analyzer. ? If we are making the code change for all plugins I think have this code in a central location helps avoid code replication

ammokhov commented 3 years ago

additionally, I dont think this is the best place to put immutability analyzer. this is invoked twice in java plugin: https://github.com/aws-cloudformation/cloudformation-cli-java-plugin/blob/master/src/main/java/software/amazon/cloudformation/AbstractWrapper.java#L201 which is to hide deserialization mismatch and the other one: https://github.com/aws-cloudformation/cloudformation-cli-java-plugin/blob/master/src/main/java/software/amazon/cloudformation/AbstractWrapper.java#L291 when we already published metrics and started request transformation

Could you help me understand why I dont think this is the best place to put immutability analyzer. ? If we are making the code change for all plugins I think have this code in a central location helps avoid code replication

this project is published in maven which is compatible only for java plugin. other plugins do not use schema project whatsoever. python plugin using different package manager, so as go. This change equates as if you made it inside java plugin

ammokhov commented 3 years ago
1. @aygold92 wondering if platform can generally handle this instead of handling this in each plugin / in each handler

2. @ammokhov this also needs to consider [`conditionallyCreateOnlyProperties`](https://github.com/aws-cloudformation/aws-cloudformation-resource-schema/issues/30) once we have those modelable

good point. i feel like if we add it to java plugin then it becomes a hard requirement on any other future plugins, which is not obvious. If we go that route then we should have a strict requirements on what plugin must satisfy, which we currently dont have. I also think this is a bit redundant and probably not necessary

This is already a hard requirement as this is asserted by Contract test: https://github.com/aws-cloudformation/cloudformation-cli/blob/master/src/rpdk/core/contract/suite/handler_update_invalid.py#L20 All this CR is doing is pushing the validation upstream in the framework vs having the check in the handlers. Also, this is also one of the handler contracts: Any createOnlyProperties specified in update handler input MUST NOT be different from their previous state.

contract tests are behavioral asserts for handlers. not plugins. plugin is a facade layer that all handlers share and we dont have any official requirements for them as of now. By adding this change we imposing a MUST implement feature on every plugin.

If we want to remove this burden from handlers then it should be done on the real upstream level - all the services that invoke handlers but not the plugin which is pretty much the same level as handlers

anshikg commented 3 years ago

Checked with stakeholders offline, This CR is redundant and these checks are being made upstream. Therefore closing this CR