feat: AddOrUpdate sets ApiVersion/Kind if not set#157
Conversation
chore: update and enhance documentation; chore: refactor resource handling methods
📝 WalkthroughWalkthroughThe PR refactors the C# SDK's resource handling API, renaming request extension methods from "Composed" to simpler names, adding a Kubernetes resource deserialization method, updating dependent code, and removing the example test project. Supporting documentation, configuration, and formatting adjustments accompany these changes. ChangesResource Handling & Deserialization Enhancements
Example Test Project Removal
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## alpha #157 +/- ##
==========================================
- Coverage 26.06% 25.78% -0.28%
==========================================
Files 7 7
Lines 188 190 +2
Branches 39 40 +1
==========================================
Hits 49 49
- Misses 137 138 +1
- Partials 2 3 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Function.SDK.CSharp/Extensions/ResponseExtensions.cs (1)
70-70:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix the locale-dependent string formatting.
The pipeline is reporting CA1305:
string.Formatbehavior could vary based on the current user's locale settings. Specify an explicitIFormatProviderto ensure consistent behavior.🌐 Proposed fix to specify culture
- Message = string.Format(message, args) + Message = string.Format(System.Globalization.CultureInfo.InvariantCulture, message, args)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Function.SDK.CSharp/Extensions/ResponseExtensions.cs` at line 70, The string.Format call in ResponseExtensions (the assignment setting Message) is locale-dependent; update it to use an explicit IFormatProvider (e.g., CultureInfo.InvariantCulture) by calling the overload that accepts an IFormatProvider (string.Format(CultureInfo.InvariantCulture, ...)) and add the necessary using for System.Globalization; locate the assignment in ResponseExtensions (the Message = string.Format(...) line) and replace it with the culture-specific overload to satisfy CA1305.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Function.SDK.CSharp/Extensions/ResourceExtensions.cs`:
- Around line 19-24: Add defensive checks and error handling to
GetKubeResource<T>: validate the extension receiver (Resource resource) and
resource.Resource_ for null and throw ArgumentNullException with clear parameter
names if null; wrap the JsonFormatter.Default.Format(...) and
KubernetesJson.Deserialize<T>(...) calls in a try-catch, catching Exception, and
rethrow a descriptive InvalidOperationException (or similar) that includes the
target type name (typeof(T)) and the original exception as innerException so
deserialization errors (malformed JSON or type mismatches) are surfaced with
context rather than causing obscure failures; do not swallow the exception and
ensure messages reference Resource.Resource_ and the failing operation
(format/deserialize).
In `@src/Function.SDK.CSharp/Models/V1ConversionReview.cs`:
- Line 20: The non-nullable property `Request` on `V1ConversionReview` triggers
CS8618 because it has no initializer; update the declaration of `public
V1ConversionReviewRequest Request { get; set; }` to ensure it is always
initialized—preferably add the `required` modifier (e.g., make it `required
V1ConversionReviewRequest Request`) so callers/deserializers must supply it;
alternatively, if it can be absent make the type nullable
(`V1ConversionReviewRequest? Request`) or provide a default using the
null-forgiving operator—choose the `required` approach for strictness.
---
Outside diff comments:
In `@src/Function.SDK.CSharp/Extensions/ResponseExtensions.cs`:
- Line 70: The string.Format call in ResponseExtensions (the assignment setting
Message) is locale-dependent; update it to use an explicit IFormatProvider
(e.g., CultureInfo.InvariantCulture) by calling the overload that accepts an
IFormatProvider (string.Format(CultureInfo.InvariantCulture, ...)) and add the
necessary using for System.Globalization; locate the assignment in
ResponseExtensions (the Message = string.Format(...) line) and replace it with
the culture-specific overload to satisfy CA1305.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 266cb6c9-be25-4a6d-be70-ef51eec09324
📒 Files selected for processing (14)
README.mdsrc/Function.SDK.CSharp.SourceGenerator/SourceGenerator.cssrc/Function.SDK.CSharp/Extensions/RequestExtensions.cssrc/Function.SDK.CSharp/Extensions/ResourceExtensions.cssrc/Function.SDK.CSharp/Extensions/ResponseExtensions.cssrc/Function.SDK.CSharp/Extensions/StateExtensions.cssrc/Function.SDK.CSharp/Models/V1ConversionReview.cssrc/Function.SDK.CSharp/Properties/launchSettings.jsontests/Function.SDK.CSharp.Example.Tests/ConversionWebhookTests.cstests/Function.SDK.CSharp.Example.Tests/Function.SDK.CSharp.Example.Tests.csprojtests/Function.SDK.CSharp.Example.Tests/TestExtensions.cstests/Function.SDK.CSharp.Example.Tests/UnitTest1.cstests/Function.SDK.CSharp.Example.Tests/xunit.runner.jsontests/Function.SDK.CSharp.Tests/V1alpha1XStorageBucket.cs
💤 Files with no reviewable changes (5)
- tests/Function.SDK.CSharp.Example.Tests/xunit.runner.json
- tests/Function.SDK.CSharp.Example.Tests/Function.SDK.CSharp.Example.Tests.csproj
- tests/Function.SDK.CSharp.Example.Tests/TestExtensions.cs
- tests/Function.SDK.CSharp.Example.Tests/ConversionWebhookTests.cs
- tests/Function.SDK.CSharp.Example.Tests/UnitTest1.cs
| public static T GetKubeResource<T>(this Resource resource) | ||
| { | ||
| var json = JsonFormatter.Default.Format(resource.Resource_); | ||
|
|
||
| return KubernetesJson.Deserialize<T>(json); | ||
| } |
There was a problem hiding this comment.
Add null checks and error handling.
The method lacks defensive checks:
- No null guard on
resourceparameter - No null check on
resource.Resource_ - No try-catch for deserialization failures (type mismatch, malformed JSON)
These omissions can cause unhandled exceptions at runtime.
🛡️ Proposed fix with null checks and error handling
public static T GetKubeResource<T>(this Resource resource)
{
+ if (resource == null)
+ throw new ArgumentNullException(nameof(resource));
+
+ if (resource.Resource_ == null)
+ throw new InvalidOperationException("Resource.Resource_ is null");
+
+ try
+ {
var json = JsonFormatter.Default.Format(resource.Resource_);
-
return KubernetesJson.Deserialize<T>(json);
+ }
+ catch (Exception ex)
+ {
+ throw new InvalidOperationException($"Failed to deserialize resource to type {typeof(T).Name}", ex);
+ }
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Function.SDK.CSharp/Extensions/ResourceExtensions.cs` around lines 19 -
24, Add defensive checks and error handling to GetKubeResource<T>: validate the
extension receiver (Resource resource) and resource.Resource_ for null and throw
ArgumentNullException with clear parameter names if null; wrap the
JsonFormatter.Default.Format(...) and KubernetesJson.Deserialize<T>(...) calls
in a try-catch, catching Exception, and rethrow a descriptive
InvalidOperationException (or similar) that includes the target type name
(typeof(T)) and the original exception as innerException so deserialization
errors (malformed JSON or type mismatches) are surfaced with context rather than
causing obscure failures; do not swallow the exception and ensure messages
reference Resource.Resource_ and the failing operation (format/deserialize).
| public string Kind { get; set; } = KubeKind; | ||
| [JsonPropertyName("request")] | ||
| public V1ConversionReviewRequest Request { get; set; } |
There was a problem hiding this comment.
Address the non-nullable property initialization warning.
The Request property is declared as non-nullable but has no default value, required modifier, or initializer. While this likely works in practice since JSON deserialization populates it, it violates C#'s nullability contracts and triggers CS8618. Consider one of these approaches:
Proposed fixes
Option 1 (recommended): Use the required modifier to enforce initialization
[JsonPropertyName("request")]
-public V1ConversionReviewRequest Request { get; set; }
+public required V1ConversionReviewRequest Request { get; set; }Option 2: Make the property nullable if it's optional
[JsonPropertyName("request")]
-public V1ConversionReviewRequest Request { get; set; }
+public V1ConversionReviewRequest? Request { get; set; }Option 3: Add a default value with null-forgiving operator
[JsonPropertyName("request")]
-public V1ConversionReviewRequest Request { get; set; }
+public V1ConversionReviewRequest Request { get; set; } = default!;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public V1ConversionReviewRequest Request { get; set; } | |
| [JsonPropertyName("request")] | |
| public required V1ConversionReviewRequest Request { get; set; } |
🧰 Tools
🪛 GitHub Actions: PR Build & Test / 0_build-test _ Build & Test.txt
[warning] 20-20: CS1591: Missing XML comment for publicly visible type or member 'V1ConversionReview.Request'.
[warning] 20-20: CS8618: Non-nullable property 'Request' must contain a non-null value when exiting constructor. Consider adding the 'required' modifier or declaring the property as nullable.
🪛 GitHub Actions: PR Build & Test / build-test _ Build & Test
[warning] 20-20: warning CS8618: Non-nullable property 'Request' must contain a non-null value when exiting constructor. Consider adding the 'required' modifier or declaring the property as nullable. (net9.0)
[warning] 20-20: warning CS1591: Missing XML comment for publicly visible type or member 'V1ConversionReview.Request' (net9.0)
[warning] 20-20: warning CS8618: Non-nullable property 'Request' must contain a non-null value when exiting constructor. Consider adding the 'required' modifier or declaring the property as nullable. (net10.0)
[warning] 20-20: warning CS1591: Missing XML comment for publicly visible type or member 'V1ConversionReview.Request' (net10.0)
🪛 GitHub Check: build-test / Build & Test
[warning] 20-20:
Non-nullable property 'Request' must contain a non-null value when exiting constructor. Consider adding the 'required' modifier or declaring the property as nullable.
[warning] 20-20:
Missing XML comment for publicly visible type or member 'V1ConversionReview.Request'
[warning] 20-20:
Non-nullable property 'Request' must contain a non-null value when exiting constructor. Consider adding the 'required' modifier or declaring the property as nullable.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Function.SDK.CSharp/Models/V1ConversionReview.cs` at line 20, The
non-nullable property `Request` on `V1ConversionReview` triggers CS8618 because
it has no initializer; update the declaration of `public
V1ConversionReviewRequest Request { get; set; }` to ensure it is always
initialized—preferably add the `required` modifier (e.g., make it `required
V1ConversionReviewRequest Request`) so callers/deserializers must supply it;
alternatively, if it can be absent make the type nullable
(`V1ConversionReviewRequest? Request`) or provide a default using the
null-forgiving operator—choose the `required` approach for strictness.
|
🎉 This PR is included in version 1.0.0-alpha.60 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
1 participant
chore: update and enhance documentation; chore: refactor resource handling methods
Summary by CodeRabbit
New Features
GetKubeResource<T>()method to deserialize Kubernetes objects from resources.Bug Fixes
GetObservedComposedResources→GetObservedResourcesandGetDesiredComposedResources→GetDesiredResourcesfor API clarity.Documentation
Tests