Enable JSON Patch in AOT #1588

hwoodiwiss · 2024-09-18T19:44:10Z

Updates KubernetesJson.Serialize with a special case for seializing V1Patch when the content is string, as based on the examples and the converter implementation, the expectation in this case is that the conent already be JSON.

fixes #1587

linux-foundation-easycla · 2024-09-18T19:44:14Z

The committers listed above are authorized under a signed CLA.

✅ login: hwoodiwiss / name: Hugo Woodiwiss (05ed1f3, 1eb62a8)

k8s-ci-robot · 2024-09-18T19:44:19Z

Welcome @hwoodiwiss!

It looks like this is your first PR to kubernetes-client/csharp 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-client/csharp has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

tg123 · 2024-09-27T16:05:21Z

src/KubernetesClient.Aot/KubernetesJson.cs

@@ -91,6 +91,11 @@ public static TValue Deserialize<TValue>(Stream json, JsonSerializerOptions json

        public static string Serialize(object value, JsonSerializerOptions jsonSerializerOptions = null)
        {
+            if (value is V1Patch { Content: string jsonValue })


i did not get it, is it still some dynamic thing?

I don't quite understand the question?

In the usages I've seen, the Content property of the V1Patch object is always set as the json string value of the patch request body. This pattern, broken down, checks:

value is of type V1Patch, and is not null
value.Content is of type string
Then it assigns the value of value.Content to jsonValue, however this is all essentially just syntactic sugar, so all of that is resolved at compile time into a slightly uglier string of conditions.

let me ask this way?
is it a breaking change or different behavior from non-aot version?
the assumption is that V1Patch must be json string?

Ahhh, got you, this isn't a breaking behaviour, and I would say it's restoring some functionality that exists in the non-aot version to the aot version.

This only restores the ability to use json string in the json patch, however I don't think full parity can be achieved without changing the API surface.

could you please also provide an example of aot json, thne i think i am good with this pr

I've added that, mostly copying the existing patch example

codecov-commenter · 2024-09-27T16:18:51Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.

Please upload report for BASE (master@541abb0). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
src/KubernetesClient.Aot/KubernetesJson.cs	0.00%	2 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff            @@
##             master    #1588   +/-   ##
=========================================
  Coverage          ?   61.43%           
=========================================
  Files             ?      103           
  Lines             ?     3073           
  Branches          ?      642           
=========================================
  Hits              ?     1888           
  Misses            ?     1185           
  Partials          ?        0

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

tg123 · 2024-11-04T23:32:52Z

/LGTM

k8s-ci-robot · 2024-11-05T05:50:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hwoodiwiss, tg123

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [tg123]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 18, 2024

k8s-ci-robot requested review from brendandburns and tg123 September 18, 2024 19:44

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Sep 18, 2024

k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Sep 18, 2024

hwoodiwiss force-pushed the fix-v1-patch branch from fb6c551 to 08159fe Compare September 18, 2024 19:45

hwoodiwiss mentioned this pull request Sep 18, 2024

KubernetesJson.Serialize fails due to null JsonSerializerInfo for V1Patch #1587

Closed

hwoodiwiss force-pushed the fix-v1-patch branch from 08159fe to e840243 Compare September 18, 2024 19:53

hwoodiwiss marked this pull request as ready for review September 19, 2024 21:11

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 19, 2024

tg123 reviewed Sep 27, 2024

View reviewed changes

hwoodiwiss added 2 commits October 23, 2024 08:40

Special case serialization of V1Patch

05ed1f3

Add AOT JSON Patch Example

1eb62a8

hwoodiwiss force-pushed the fix-v1-patch branch from e840243 to 1eb62a8 Compare October 23, 2024 07:51

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 23, 2024

k8s-ci-robot assigned tg123 Nov 4, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 4, 2024

tg123 approved these changes Nov 5, 2024

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 5, 2024

k8s-ci-robot merged commit c3b3a08 into kubernetes-client:master Nov 5, 2024
14 checks passed

hwoodiwiss deleted the fix-v1-patch branch November 5, 2024 16:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable JSON Patch in AOT #1588

Enable JSON Patch in AOT #1588

hwoodiwiss commented Sep 18, 2024 •

edited

Loading

linux-foundation-easycla bot commented Sep 18, 2024 •

edited

Loading

k8s-ci-robot commented Sep 18, 2024

tg123 Sep 27, 2024

hwoodiwiss Sep 27, 2024

tg123 Sep 27, 2024

hwoodiwiss Sep 27, 2024 •

edited

Loading

tg123 Oct 21, 2024

hwoodiwiss Oct 23, 2024

codecov-commenter commented Sep 27, 2024 •

edited

Loading

tg123 commented Nov 4, 2024

k8s-ci-robot commented Nov 5, 2024

Enable JSON Patch in AOT #1588

Enable JSON Patch in AOT #1588

Conversation

hwoodiwiss commented Sep 18, 2024 • edited Loading

linux-foundation-easycla bot commented Sep 18, 2024 • edited Loading

k8s-ci-robot commented Sep 18, 2024

tg123 Sep 27, 2024

Choose a reason for hiding this comment

hwoodiwiss Sep 27, 2024

Choose a reason for hiding this comment

tg123 Sep 27, 2024

Choose a reason for hiding this comment

hwoodiwiss Sep 27, 2024 • edited Loading

Choose a reason for hiding this comment

tg123 Oct 21, 2024

Choose a reason for hiding this comment

hwoodiwiss Oct 23, 2024

Choose a reason for hiding this comment

codecov-commenter commented Sep 27, 2024 • edited Loading

Codecov Report

tg123 commented Nov 4, 2024

k8s-ci-robot commented Nov 5, 2024

hwoodiwiss commented Sep 18, 2024 •

edited

Loading

linux-foundation-easycla bot commented Sep 18, 2024 •

edited

Loading

hwoodiwiss Sep 27, 2024 •

edited

Loading

codecov-commenter commented Sep 27, 2024 •

edited

Loading