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

Enable JSON Patch in AOT #1588

Merged
merged 2 commits into from
Nov 5, 2024
Merged

Enable JSON Patch in AOT #1588

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
05ed1f3
Special case serialization of V1Patch
hwoodiwiss Sep 18, 2024
1eb62a8
Add AOT JSON Patch Example
hwoodiwiss Oct 23, 2024
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
32 changes: 32 additions & 0 deletions examples/patch-aot/Program.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
using k8s;

Check warning on line 1 in examples/patch-aot/Program.cs

View workflow job for this annotation

GitHub Actions / Analyze (csharp) 

The file header is missing or not located at the top of the file. (https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1633.md)

Check warning on line 1 in examples/patch-aot/Program.cs

View workflow job for this annotation

GitHub Actions / Analyze (csharp)

using k8s.Models;

Check warning on line 2 in examples/patch-aot/Program.cs

View workflow job for this annotation

GitHub Actions / Analyze (csharp)


var config = KubernetesClientConfiguration.BuildConfigFromConfigFile();
IKubernetes client = new Kubernetes(config);
Console.WriteLine("Starting Request!");

var pod = client.CoreV1.ListNamespacedPod("default").Items.First();
var name = pod.Metadata.Name;
PrintLabels(pod);

var patchStr = @"
{
""metadata"": {
""labels"": {
""test"": ""test""
}
}
}";

client.CoreV1.PatchNamespacedPod(new V1Patch(patchStr, V1Patch.PatchType.MergePatch), name, "default");
PrintLabels(client.CoreV1.ReadNamespacedPod(name, "default"));

static void PrintLabels(V1Pod pod)
{
Console.WriteLine($"Labels: for {pod.Metadata.Name}");
foreach (var (k, v) in pod.Metadata.Labels)
{
Console.WriteLine($"{k} : {v}");
}

Check warning on line 30 in examples/patch-aot/Program.cs

View workflow job for this annotation

GitHub Actions / Analyze (csharp)

Console.WriteLine("=-=-=-=-=-=-=-=-=-=-=");
}
11 changes: 11 additions & 0 deletions examples/patch-aot/patch-aot.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<OutputType>Exe</OutputType>
<ImplicitUsings>enable</ImplicitUsings>
<Nullable>enable</Nullable>
<PublishAot>true</PublishAot>
</PropertyGroup>
<ItemGroup>
<ProjectReference Include="..\..\src\KubernetesClient.Aot\KubernetesClient.Aot.csproj"/>
</ItemGroup>
</Project>
5 changes: 5 additions & 0 deletions src/KubernetesClient.Aot/KubernetesJson.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 })
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@hwoodiwiss hwoodiwiss Sep 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

{
return jsonValue;
}

var info = SourceGenerationContext.Default.GetTypeInfo(value.GetType());
return JsonSerializer.Serialize(value, info);
}
Expand Down
Loading