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

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
adb66a7
Adding ConditionalCreateOnlyProperties to aws resource type schema
willdavsmith May 3, 2023
a7c9eed
Adding unit tests
willdavsmith May 4, 2023
420059d
Merge remote-tracking branch 'origin/main' into willdavsmith/aws-patc…
willdavsmith May 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 10 additions & 8 deletions pkg/aws/operations/operations.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,11 @@ import (
)

type ResourceTypeSchema struct {
Properties map[string]any `json:"properties,omitempty"`
ReadOnlyProperties []string `json:"readOnlyProperties,omitempty"`
CreateOnlyProperties []string `json:"createOnlyProperties,omitempty"`
WriteOnlyProperties []string `json:"writeOnlyProperties,omitempty"`
Properties map[string]any `json:"properties,omitempty"`
ReadOnlyProperties []string `json:"readOnlyProperties,omitempty"`
CreateOnlyProperties []string `json:"createOnlyProperties,omitempty"`
ConditionalCreateOnlyProperties []string `json:"conditionalCreateOnlyProperties,omitempty"`
Comment on lines +21 to +22
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!

WriteOnlyProperties []string `json:"writeOnlyProperties,omitempty"`
}

// FlattenProperties flattens a state object.
Expand Down Expand Up @@ -143,11 +144,12 @@ func GeneratePatch(currentState []byte, desiredState []byte, schema []byte) (jso
if isWriteOnlyProperty && isCreateOnlyProperty {
flattenedDesiredStateObject[k] = v
} else if _, exists := flattenedDesiredStateObject[k]; !exists {
// Add the property (if not exists already) to the desired state if it is a read-only or create-only
// property. This ensures that these types of properties result in a no-op in the patch if they aren't
// updated in the desired state
// Add the property (if not exists already) to the desired state if it is a read-only, create-only,
// or conditional-create-only property. This ensures that these types of properties result in a
// no-op in the patch if they aren't updated in the desired state
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?

flattenedDesiredStateObject[k] = v
}
}
Expand Down
41 changes: 41 additions & 0 deletions pkg/aws/operations/operations_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,6 +265,47 @@ func Test_GeneratePatch(t *testing.T) {
},
nil,
},
{
"conditional-create-only-property noops if not updated",
map[string]any{
"A": "B",
},
map[string]any{},
map[string]any{
"properties": map[string]any{
"A": map[string]any{},
},
"conditionalCreateOnlyProperties": []any{
"/properties/A",
},
},
nil,
},
{
"can update conditional-create-only-property",
map[string]any{
"A": "B",
},
map[string]any{
"A": "C",
},
map[string]any{
"properties": map[string]any{
"A": map[string]any{},
},
"conditionalCreateOnlyProperties": []any{
"/properties/A",
},
},
jsondiff.Patch{
{
Type: "replace",
Path: "/A",
OldValue: "B",
Value: "C",
},
},
},
}

for _, testCase := range testCases {
Expand Down