Skip default delete #2456
Merged
Skip default delete #2456
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
danielrbradley
force-pushed
the
skip-default-delete
branch
from
759dae4
to
47ade26
Compare
Does the PR have any schema changes?Looking good! No breaking changes found. |
thomas11
reviewed
May 16, 2023
thomas11
approved these changes
May 16, 2023
Expose only a method rather than the internal data structures.
- Use nil to indicate we don't have a default state defined instead of HasDefault. - Expose standalone SkipDeleteOperation function for the provider. - Call into defaults function directly from the provider rather than serializing more properties into the metadata. - We can't skip serializing the DefaultBody as there's some cases where it's implicitly set based on a `/list` path existing. - Panic if we define duplicate default resource states (after normalization).
danielrbradley
force-pushed
the
skip-default-delete
branch
from
fb63ddf
to
045090a
Compare
|
Is this a breaking behavior change? Have we documented it anywhere? How did we decide it's okay to ship it? Is it temporary until a better fix lands? What is our principle how to decide for which resources this is okay to apply? |
| }, | ||
| HasNonEmptyCollections: true, | ||
| } | ||
| assert.Equal(t, &expected, actual) |
There was a problem hiding this comment.
Could you please explain what we are testing here? GetDefaultResourceState does two things:
- Calls
NormalizePath - Calls a map lookup and returns the value or default
What do we get by having 5 tests for it? It feels like we are testing the contents of defaultResourcesStateNormalized?
thomas11
added a commit
that referenced
this pull request
Feb 14, 2024
…#3076) This PR fixes what I believe is a bug introduced in #2456. For the default state of a resource, we need to distinguish between `nil` (no default) and the empty object as the empty default. This is also described in the doc comment of `AzureAPIResource.DefaultBody` in resources.go: > - omitempty is not set to distinct between nil (no default) and empty maps (empty default) `AzureClient.CanCreate` checks `DefaultBody != nil` to detect resources that are automatically created with their parent. The actual change in metadata is not visible in the PR because we don't commit it. It looks like this: ```diff --- ../m-master.json 2024-02-12 20:22:54.578408624 +0100 +++ ../m.json 2024-02-12 20:21:14.881957652 +0100 @@ -1005922,7 +1005922,7 @@ ] } }, - "defaultBody": {}, + "defaultBody": null, "putAsyncStyle": "azure-async-operation", "deleteAsyncStyle": "azure-async-operation" }, ```
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
For resources without DELETE operations defined (only PUT) and no obvious "default" state to reset to, add the option for delete to be a no-op.
This is required for Sql database TransparentDataEncryption which requires different properties to be set for different API versions. This could be solved by introducing version awareness, but that would be a larger piece of work.
We model skipping the delete by setting the "default body" to nil.
This includes a refactor to disentwine path normalisation and default resource state from the main discovery process. These are moved into their own modules so only public members can be interacted with - to avoid interacting with the wrong internal parts.