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.

Sign up for GitHub

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

Jump to bottom

removing contract_update_create_only_property #721

Merged
merged 1 commit into from
Apr 1, 2021
Merged

removing contract_update_create_only_property #721

merged 1 commit into from
Apr 1, 2021

Conversation

PatMyron
Copy link
Contributor

@PatMyron PatMyron commented Apr 1, 2021 •
edited
Loading

@PatMyron PatMyron marked this pull request as ready for review April 1, 2021 01:22
ammokhov
ammokhov reviewed Apr 1, 2021
View reviewed changes
Copy link
Contributor

@ammokhov ammokhov left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I think it's a good point. We intend to have schema as a single source of truth. Handlers really should not throw not updatable exception on immutable change. This defeats the purpose of the field. I am ok removing this tests.

@aygold92 @jotompki @wbingli thoughts?

@ammokhov ammokhov requested a review from anshikg April 1, 2021 02:07
anshikg
anshikg suggested changes Apr 1, 2021
View reviewed changes
Copy link
Contributor

@anshikg anshikg left a comment

Choose a reason for hiding this comment

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

I agree we need to remove this contract test. Lets remove this once we have closure on how we want to handle this check in the platform: aws-cloudformation/cloudformation-resource-schema#120

@aygold92
Copy link
Contributor

aygold92 commented Apr 1, 2021

I think it's a good point. We intend to have schema as a single source of truth. Handlers really should not throw not updatable exception on immutable change. This defeats the purpose of the field. I am ok removing this tests.

@aygold92 @jotompki @wbingli thoughts?

Right. Once we introduce conditionalCreateOnlyProperties, we should have essentially an identical contract test for that to replace this one.

@anshikg anshikg merged commit ad084f8 into master Apr 1, 2021
@PatMyron PatMyron deleted the contract branch April 1, 2021 22:23
eduardomourar pushed a commit to eduardomourar/cloudformation-cli that referenced this pull request Apr 30, 2021
@PatMyron @eduardomourar
removing contract_update_create_only_property (aws-cloudformation#721)
c8ba6cb
kddejong pushed a commit to kddejong/cloudformation-cli that referenced this pull request Oct 24, 2022
@PatMyron @kddejong
removing contract_update_create_only_property (aws-cloudformation#721)
e071204
kddejong pushed a commit to kddejong/cloudformation-cli that referenced this pull request Oct 24, 2022
@PatMyron @kddejong
removing contract_update_create_only_property (aws-cloudformation#721)
89c1af1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ammokhov ammokhov ammokhov left review comments

@aygold92 aygold92 aygold92 approved these changes

@anshikg anshikg anshikg approved these changes

@johnttompkins johnttompkins Awaiting requested review from johnttompkins

@kddejong kddejong Awaiting requested review from kddejong

@miparnisari miparnisari Awaiting requested review from miparnisari

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@PatMyron @aygold92 @anshikg @ammokhov