Skip to content
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

Adding conditionalCreateOnlyProperties to metaschema #121

Merged
merged 2 commits into from
Mar 29, 2021

Conversation

ammokhov
Copy link
Contributor

@ammokhov ammokhov commented Mar 26, 2021

Issue #, if available: #30

Description of changes:

Adding conditionallyCreateOnlyProperties to meta schema, so that resource providers could model properties that could not deterministically predict replacement/inline update

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 Mar 26, 2021
@ammokhov ammokhov added the enhancement New feature or request label Mar 26, 2021
@ammokhov ammokhov requested a review from PatMyron March 26, 2021 05:45
@@ -387,6 +387,10 @@
"description": "A list of JSON pointers to properties (typically sensitive) that are able to be specified by the customer but unable to be returned in a Read request",
"$ref": "#/definitions/jsonPointerArray"
},
"conditionallyCreateOnlyProperties": {
"description": "A list of JSON pointers to properties that are only able to cause replacement during the Update request.",
Copy link
Contributor

@PatMyron PatMyron Mar 26, 2021

Choose a reason for hiding this comment

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

think only might be misleading. Also, do you think it's worth adding an example?

Suggested change
"description": "A list of JSON pointers to properties that are only able to cause replacement during the Update request.",
"description": "A list of JSON pointers to properties that may or may not cause replacement during the Update request. For example, a version property might be upgradable in-place but downgrading may require resource replacement",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You think we should have a specific example like version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jotompki @wbingli @aygold92 @anshikg thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would even give a specific example, like that for EBS volumes you can increase the size property, but decreasing it would require replacement https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/modify-volume-requirements.html#elastic-volumes-limitations

That being said, I would not even mention replacement, or if so mention that specifically as a cloudformation specific thing. Cloud API will never replace your resource, it would just fail with NotUpdatable exception. I would say something like:

A list of JSON pointers for properties that can only be updated under certain conditions.  For example, you can increase the size of an EBS volume but you cannot decrease it.  When updating this property for a resource in a CloudFormation stack, the resource will be replaced if it cannot be updated.

Copy link
Contributor

Choose a reason for hiding this comment

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

One consideration about specific examples is that they may change in the future. EBS might remove the restriction on decreasing size in-place eventually

Copy link
Contributor

@aygold92 aygold92 Mar 29, 2021

Choose a reason for hiding this comment

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

true, I'm fine with the generic example you had above, but again be careful about using "replacement"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

how about using rds? db downgrade is unlikely to change over time:

A list of JSON pointers for properties that can only be updated under certain conditions.  For example, you can upgrade the engine version of a RDS DBInstance but you cannot downgrade it.  When updating this property for a resource in a CloudFormation stack, the resource will be replaced if it cannot be updated.

@benkehoe
Copy link

Could I suggest these be called conditionalCreateOnlyProperties? Adverbs in field names are pretty uncommon, and as a user I think I would often forget that it was conditionallyCreateOnlyProperties. It's a create only property that is conditional, rather than a property that is conditionally create only

Copy link
Contributor

@johnttompkins johnttompkins left a comment

Choose a reason for hiding this comment

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

Looks good pending the change to the description

@aygold92
Copy link
Contributor

aygold92 commented Mar 29, 2021

Could I suggest these be called conditionalCreateOnlyProperties? Adverbs in field names are pretty uncommon, and as a user I think I would often forget that it was conditionallyCreateOnlyProperties. It's a create only property that is conditional, rather than a property that is conditionally create only

I at first thought to call it conditionalCreateOnlyProperties, but I'm not sure what the distinction is between "a create only property that is conditional" and "a property that is conditionally create only". Actually, it seems more like the second one in this case. I'd personally be fine calling it either one

@benkehoe
Copy link

My main point is that it seems unusually phrased, and thus easy to forget/get confused. But in my mind, it's a create-only property, that isn't always triggered, as opposed to be only sometimes create-only.

@ammokhov ammokhov merged commit 7d82244 into aws-cloudformation:master Mar 29, 2021
@ammokhov ammokhov deleted the conditional-immutability branch March 29, 2021 20:57
@benkehoe
Copy link

Thanks for making the change! I saw the merge notification, but not the new commit. When I talked with TAM about the new feature, I called it conditionallyCreateOnlyProperties and they echoed back conditionalCreateOnlyProperties, which I think is evidence that it would have caused confusion.

@PatMyron PatMyron changed the title Adding conditionallyCreateOnlyProperties to metaschema Adding conditionalCreateOnlyProperties to metaschema Apr 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants