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

Adding ConditionalCreateOnlyProperties handling to AWS patch logic #5512

Merged
merged 3 commits into from
May 5, 2023
Merged

Adding ConditionalCreateOnlyProperties handling to AWS patch logic #5512

merged 3 commits into from
May 5, 2023

Conversation

willdavsmith
Copy link
Contributor

@willdavsmith willdavsmith commented May 3, 2023 •
edited
Loading

Description

  • Adding check for ConditionalCreateOnlyProperties in AWS Diff code
  • FIxes a bug with AWS RDSInstance redeployments

Issue reference

Fixes: #issue_number

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

  • Code compiles correctly
  • Adds necessary unit tests for change
  • Adds necessary E2E tests for change
  • Unit tests passing
  • Extended the documentation / Created issue for it

@willdavsmith willdavsmith requested a review from a team as a code owner May 3, 2023 18:27
@github-actions
Copy link

github-actions bot commented May 3, 2023 •
edited
Loading

Test Results

2 516 tests  +2   2 509 ✔️ +2   1m 55s ⏱️ ±0s
   230 suites ±0          7 💤 ±0 
       1 files   ±0          0 ❌ ±0 

Results for commit 420059d. ± Comparison against base commit 42a6f1b.

This pull request removes 2 and adds 4 tests. Note that renamed tests count towards both. 
github.com/project-radius/radius/pkg/armrpc/frontend/controller ‑ TestValidateEtag_IfMatch/430687f4-6faf-424d-a547-95efbaa122ba
github.com/project-radius/radius/pkg/armrpc/frontend/controller ‑ TestValidateEtag_IfMatch/430687f4-6faf-424d-a547-95efbaa122ba#01

github.com/project-radius/radius/pkg/armrpc/frontend/controller ‑ TestValidateEtag_IfMatch/4966d0ae-b2e0-46c9-9267-4e159eba887e
github.com/project-radius/radius/pkg/armrpc/frontend/controller ‑ TestValidateEtag_IfMatch/4966d0ae-b2e0-46c9-9267-4e159eba887e#01
github.com/project-radius/radius/pkg/aws/operations ‑ Test_GeneratePatch/can_update_conditional-create-only-property
github.com/project-radius/radius/pkg/aws/operations ‑ Test_GeneratePatch/conditional-create-only-property_noops_if_not_updated

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented May 3, 2023

62.1

For the detailed report, please go to Checks tab, click Build and Test, and then download unit_test_coverage artifact at the bottom of build page.

  • Your PR branch coverage: 62.1 %
  • main branch coverage: 62.1 %
  • diff coverage: 0 %

The coverage result does not include the functional test coverage.

ytimocin
ytimocin reviewed May 3, 2023
View reviewed changes
pkg/aws/operations/operations.go
Comment on lines +21 to +22
CreateOnlyProperties []string `json:"createOnlyProperties,omitempty"`
ConditionalCreateOnlyProperties []string `json:"conditionalCreateOnlyProperties,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think there is a way to somehow merge these two?

Copy link
Contributor

Choose a reason for hiding this comment

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

Like another struct that would be named CreateOnlyProperty which can have a conditional field on it that could be a boolean? And the CreateOnlyProperties could be an array of CreateOnlyProperty. Is it too much of an unnecessary effort? I am just brainstorming and this suggestion is not that important as of now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ConditionalCreateOnlyProperties is a specific reserved keyword in AWS CloudFormation specs:
image

This struct is used during parsing of the resource type schemas. IMO the more clear logic is to keep them separate here but definitely open to suggestions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Didn't know this! Thanks!

@ytimocin
Copy link
Contributor

ytimocin commented May 3, 2023

may need a rebase other than that LGTM

youngbupark
youngbupark reviewed May 3, 2023
View reviewed changes
pkg/aws/operations/operations.go
isReadOnlyProperty := slices.Contains(resourceTypeSchema.ReadOnlyProperties, property)
if isReadOnlyProperty || isCreateOnlyProperty {
isConditionalCreateOnlyProperty := slices.Contains(resourceTypeSchema.ConditionalCreateOnlyProperties, property)
if isReadOnlyProperty || isCreateOnlyProperty || isConditionalCreateOnlyProperty {
Copy link

Choose a reason for hiding this comment

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

Can you please add unit-test or functional test?

@willdavsmith willdavsmith requested a review from youngbupark May 4, 2023 16:53
@github-actions
Copy link

github-actions bot commented May 4, 2023

62.1

For the detailed report, please go to Checks tab, click Build and Test, and then download unit_test_coverage artifact at the bottom of build page.

  • Your PR branch coverage: 62.1 %
  • main branch coverage: 62.1 %
  • diff coverage: 0 %

The coverage result does not include the functional test coverage.

youngbupark
youngbupark approved these changes May 4, 2023
View reviewed changes
Copy link

@youngbupark youngbupark left a comment

Choose a reason for hiding this comment

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

👍

@willdavsmith willdavsmith merged commit 39151ce into main May 5, 2023
@willdavsmith willdavsmith deleted the willdavsmith/aws-patch-update branch May 5, 2023 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@youngbupark youngbupark youngbupark approved these changes

@ytimocin ytimocin ytimocin approved these changes

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

Successfully merging this pull request may close these issues.
3 participants
@willdavsmith @ytimocin @youngbupark