Add support for explicitEmptyCollections payloadType #3540
Merged
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
nojnhuh
reviewed
Nov 10, 2023
| @@ -418,6 +418,8 @@ func (c *armTypeCreator) createARMProperty( | |||
| // See https://azure.github.io/azure-service-operator/design/adr-2023-04-patch-collections/ for how we're solving it. | |||
| result = result.WithoutTag("json", "omitempty") | |||
|
|
|||
| case config.ExplicitEmptyCollections: | |||
| fallthrough | |||
There was a problem hiding this comment.
don't see too many of these lol
theunrepentantgeek
approved these changes
Nov 10, 2023
v2/api/containerservice/v1api20210501/managed_cluster_types_gen.go
Outdated
Show resolved
Hide resolved
v2/tools/generator/internal/codegen/pipeline/create_arm_types.go
Outdated
Show resolved
Hide resolved
v2/tools/generator/internal/astmodel/conversion_function_builder.go
Outdated
Show resolved
Hide resolved
v2/tools/generator/internal/astmodel/conversion_function_builder.go
Outdated
Show resolved
Hide resolved
| codeGenerationContext: codeGenerationContext, | ||
| }, | ||
| typeConversionBuilder: astmodel.NewConversionFunctionBuilder(c.idFactory, codeGenerationContext), | ||
| typeConversionBuilder: astmodel.NewConversionFunctionBuilder(c.idFactory, codeGenerationContext, false), |
There was a problem hiding this comment.
Is there a better option than passing a mysterious false here?
There was a problem hiding this comment.
I've changed this somewhat so I think it's clearer
b8d786f to
f57b138
Compare
Codecov Report
@@ Coverage Diff @@
## main #3540 +/- ##
==========================================
- Coverage 54.04% 53.99% -0.05%
==========================================
Files 1573 1578 +5
Lines 656726 657622 +896
==========================================
+ Hits 354905 355103 +198
- Misses 244338 244925 +587
- Partials 57483 57594 +111
|
|
/ok-to-test sha=f57b138 |
1 similar comment
|
/ok-to-test sha=f57b138 |
This fixes Azure#3522. * Use this new payloadType for AKS resources, because the AKS RP doesn't treat "myProperty": null any different than omission of "myProperty" entirely, which meaans that it doesn't correctly clear properties that it should. * Update test recordings.
f57b138 to
a5401ec
Compare
|
/ok-to-test sha=a5401ec |
theunrepentantgeek
approved these changes
Nov 12, 2023
| @@ -66,21 +66,101 @@ interactions: | |||
| code: 200 | |||
| duration: "" | |||
| - request: | |||
| body: '{"location":"westus2","name":"asotest-keyvault-ngmgjs","properties":{"accessPolicies":[{"applicationId":"00000000-0000-0000-0000-000000000000","objectId":"00000000-0000-0000-0000-000000000000","permissions":{"certificates":["get"],"keys":["get"],"secrets":["get"],"storage":["get"]},"tenantId":"00000000-0000-0000-0000-000000000000"}],"enableSoftDelete":false,"sku":{"family":"A","name":"standard"},"tenantId":"00000000-0000-0000-0000-000000000000"}}' | |||
| body: "" | |||
There was a problem hiding this comment.
Is this payload change expected?
There was a problem hiding this comment.
It's for a different URL than before (though it doesn't look it, if you see down on line 76 the URL is new), so I think the answer is yes.
GET shouldn't have a request body, so this is right.
This is probably because I changed some of the KV-using tests to purge+create so that they didn't need a manual step to start.
|
/ok-to-test sha=a5401ec |
super-harsh
approved these changes
Nov 12, 2023
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.
This fixes #3522.
How does this PR make you feel:
If applicable: