Conversation
…cies and configuration
Reviewer's GuideThis pull request introduces two new projects—Abstractions and Hosting—into the solution, updates global configuration to include them, and implements initial Orleans-based hosting extension methods along with a constants class for runtime instrumentation. Class diagram for new HostApplicationBuilderExtensions in Hosting projectclassDiagram
class HostApplicationBuilderExtensions {
+HostApplicationBuilder UseRuntimeServer(HostApplicationBuilder builder)
-HostApplicationBuilder AddEventSourcingDefaults(HostApplicationBuilder builder)
-HostApplicationBuilder AddHostingDefaults(HostApplicationBuilder builder)
-HostApplicationBuilder AddActivityPropagation(HostApplicationBuilder builder)
}
class HostApplicationBuilder
HostApplicationBuilderExtensions --|> HostApplicationBuilder: extends via extension methods
Class diagram for new Constants class in Abstractions projectclassDiagram
class Constants {
}
class Runtime {
}
class Server {
+const string Name
+static readonly ActivitySource Source
}
Constants o-- Runtime
Runtime o-- Server
Server ..> ActivitySource: uses
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
WalkthroughIntroduces two new projects (Abstractions and Hosting), adds an Orleans bootstrapping extension UseRuntimeServer, provides runtime tracing constants, and updates solution and configuration files (.editorconfig, .globalconfig, Directory.*.props, package locks). Also adjusts package settings and public API analyzer item definitions. Changes
Sequence Diagram(s)sequenceDiagram
actor App
participant Builder as HostApplicationBuilder
participant Ext as HostApplicationBuilderExtensions
participant Orleans as Orleans SiloBuilder
participant Config as IConfiguration
App->>Ext: UseRuntimeServer(builder)
Ext->>Builder: AddEventSourcingDefaults()
Builder->>Orleans: UseOrleans(...AddLog/StateStorageConsistency)
Ext->>Builder: AddHostingDefaults()
Builder->>Config: Read "Escendit:Runtime:Server:Hosting"
alt Hosting == "Kubernetes"
Builder->>Orleans: UseKubernetesHosting()
end
Ext->>Builder: AddActivityPropagation()
Builder->>Orleans: AddActivityPropagation()
Ext-->>App: HostApplicationBuilder
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @snovak7 - I've reviewed your changes - here's some feedback:
- Consolidate the multiple UseOrleans calls into a single pipeline to avoid reconfiguring the silo builder multiple times.
- In AddHostingDefaults, change the switch to return and apply the updated siloBuilder instead of assigning to discard so UseKubernetesHosting actually takes effect.
- Consider flattening or reorganizing the nested static classes in Constants to improve readability and avoid suppressing CA1034.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consolidate the multiple UseOrleans calls into a single pipeline to avoid reconfiguring the silo builder multiple times.
- In AddHostingDefaults, change the switch to return and apply the updated siloBuilder instead of assigning to discard so UseKubernetesHosting actually takes effect.
- Consider flattening or reorganizing the nested static classes in Constants to improve readability and avoid suppressing CA1034.
## Individual Comments
### Comment 1
<location> `src/Abstractions/Properties/AssemblyInfo.cs:6` </location>
<code_context>
+
+using System.Runtime.InteropServices;
+
+[assembly: CLSCompliant(false)]
+[assembly: ComVisible(false)]
</code_context>
<issue_to_address>
Consider documenting the rationale for disabling CLS compliance and COM visibility.
If this is a public API, please clarify why these attributes are set, as it may affect library consumers and future maintenance.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
src/Hosting/Properties/AssemblyInfo.cs (1)
1-4: Add ComVisible(false) and explicit usings for consistency and resilienceFor parity with Abstractions and to avoid accidental COM exposure, add ComVisible(false). Also consider adding explicit using System; (and System.Runtime.InteropServices) to avoid relying on ImplicitUsings.
Apply:
+using System; +using System.Runtime.InteropServices; [assembly: CLSCompliant(false)] +[assembly: ComVisible(false)]src/Abstractions/Properties/AssemblyInfo.cs (1)
4-7: Add explicit using System; to ensure CLSCompliant resolves even if ImplicitUsings changeIf ImplicitUsings are disabled later, CLSCompliant won’t resolve. Add using System; explicitly.
using System.Runtime.InteropServices; +using System; [assembly: CLSCompliant(false)] [assembly: ComVisible(false)]src/Abstractions/Constants.cs (1)
28-33: Optional: consider an alternative to nested public types to avoid CA1034 suppressionIf API design allows, flatten the type hierarchy (e.g., Escendit.Extensions.Hosting.Abstractions.RuntimeServerConstants) to avoid disabling CA1034. Otherwise, the current approach is acceptable.
src/Hosting/HostApplicationBuilderExtensions.Orleans.cs (3)
58-66: Make hosting mode check case-insensitive and avoid discard of the switch resultEnsure "kubernetes" works regardless of casing, and call the method directly for clarity.
- var hostingMode = builder - .Configuration - .GetValue<string>("Hosting"); - - _ = hostingMode switch - { - "Kubernetes" => siloBuilder.UseKubernetesHosting(), - _ => siloBuilder, - }; + var hostingMode = builder.Configuration.GetValue<string>("Hosting"); + if (string.Equals(hostingMode, "Kubernetes", StringComparison.OrdinalIgnoreCase)) + { + siloBuilder.UseKubernetesHosting(); + }Note: add using System; if not already available via ImplicitUsings.
71-76: Consistency: add ArgumentNullException.ThrowIfNull(builder) like other helpersMaintain consistent guard clauses across helpers.
private static HostApplicationBuilder AddActivityPropagation(this HostApplicationBuilder builder) { - return builder + ArgumentNullException.ThrowIfNull(builder); + return builder .UseOrleans(siloBuilder => siloBuilder .AddActivityPropagation()); }
51-54: Scope binding for ClusterOptions to a section to avoid over-bindingBinding the entire root config may bind unintended keys. Consider scoping to a section (e.g., "Orleans:ClusterOptions").
- .Configure<ClusterOptions>(configureOptions => builder - .Configuration - .Bind(configureOptions)); + .Configure<ClusterOptions>( + builder.Configuration.GetSection("Orleans:ClusterOptions").Bind);src/Abstractions/Escendit.Extensions.Hosting.Abstractions.csproj (1)
1-1: Consider adding minimal project configuration for clarity.This project file is extremely minimal. For an Abstractions project, consider adding basic configuration to make the purpose clearer and ensure consistency:
-<Project Sdk="Microsoft.NET.Sdk" /> +<Project Sdk="Microsoft.NET.Sdk"> + <!-- This project inherits common configuration from Directory.Build.props --> + <!-- Contains abstractions and shared constants for hosting extensions --> +</Project>While the current approach works (inheriting everything from Directory.Build.props), adding a comment clarifies the intentional minimal nature and documents the project's purpose.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
.editorconfig(1 hunks).globalconfig(1 hunks)Directory.Build.props(1 hunks)Directory.Packages.props(0 hunks)Solution.slnx(1 hunks)src/Abstractions/Constants.cs(1 hunks)src/Abstractions/Escendit.Extensions.Hosting.Abstractions.csproj(1 hunks)src/Abstractions/Properties/AssemblyInfo.cs(1 hunks)src/Abstractions/PublicAPI.Shipped.txt(1 hunks)src/Abstractions/packages.lock.json(1 hunks)src/Hosting/Escendit.Extensions.Hosting.csproj(1 hunks)src/Hosting/HostApplicationBuilderExtensions.Orleans.cs(1 hunks)src/Hosting/Properties/AssemblyInfo.cs(1 hunks)src/Hosting/PublicAPI.Shipped.txt(1 hunks)src/Hosting/packages.lock.json(1 hunks)
💤 Files with no reviewable changes (1)
- Directory.Packages.props
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/Abstractions/Properties/AssemblyInfo.cs (1)
src/Abstractions/Constants.cs (1)
Runtime(18-35)
🔇 Additional comments (9)
.editorconfig (1)
25-25: LGTM: ReSharper namespace highlighting disabled for C#This aligns with the global SA1135 disable and avoids noisy namespace ordering warnings across the new projects.
.globalconfig (1)
6-8: LGTM: SA1135 disabled globallyMatches the repo’s direction to relax namespace ordering checks; keeps build noise down without affecting runtime behavior.
src/Hosting/PublicAPI.Shipped.txt (1)
1-1: Ignore Unshipped API file suggestion for Hosting
There are no new public types or extension methods in thesrc/Hostingfolder, so noPublicAPI.Unshipped.txtentry is needed.Likely an incorrect or invalid review comment.
src/Abstractions/Constants.cs (1)
4-10: Fix compile error: using directives must precede file-scoped namespaceusing System.Diagnostics; appears after a file-scoped namespace; that’s invalid C#. Move the using above the namespace (or convert to a block-scoped namespace).
Apply:
-namespace Escendit.Extensions.Hosting.Abstractions; - -#pragma warning disable CA1034 - -using System.Diagnostics; +using System.Diagnostics; + +namespace Escendit.Extensions.Hosting.Abstractions; + +#pragma warning disable CA1034Likely an incorrect or invalid review comment.
src/Hosting/Escendit.Extensions.Hosting.csproj (1)
1-12: LGTM! Well-structured Orleans hosting project.The package selection excellently supports the PR objective of providing Orleans server hosting defaults:
Microsoft.Orleans.Serverprovides core Orleans server functionalityMicrosoft.Orleans.EventSourcingenables event sourcing capabilitiesMicrosoft.Orleans.Hosting.Kubernetessupports Kubernetes deploymentsMicrosoft.Extensions.Http.ResilienceandMicrosoft.Extensions.ServiceDiscoveryadd modern service communication patternsThe ProjectReference to the Abstractions project is correctly configured, and omitting version numbers is appropriate given the central package management configuration.
Directory.Build.props (1)
64-65: LGTM! Correct change for new projects with Public API tracking.Changing from
UpdatetoIncludeis appropriate when adding new projects that will have their ownPublicAPI.Shipped.txtandPublicAPI.Unshipped.txtfiles. TheUpdateattribute modifies existing items, whileIncludeadds new items to the build, which is what's needed for the new Abstractions and Hosting projects.Solution.slnx (1)
2-8: LGTM! Proper solution structure for the new projects.The solution file correctly adds both new projects under the
/src/folder with appropriate organization. The folder structure is clean and the project paths are correctly specified.src/Hosting/packages.lock.json (1)
1-1074: LGTM! Comprehensive package lock file ensures reproducible builds.This lock file properly captures all dependencies for the Hosting project with:
- Consistent Orleans 9.2.1 versions across all Orleans packages
- Appropriate Microsoft.Extensions package versions
- Content hashes for security and integrity verification
- Proper inclusion of the Abstractions project as a Project reference
The extensive dependency tree is expected for Orleans-based projects due to the framework's comprehensive feature set.
src/Abstractions/packages.lock.json (1)
1-58: LGTM! Minimal and appropriate lock file for an Abstractions project.This lock file correctly contains only development-time dependencies:
- Code analysis tools (SonarAnalyzer, Roslynator, Meziantou.Analyzer)
- Escendit tooling packages for branding and analysis
- PublicAPI analyzers for API surface tracking
The minimal dependency footprint is appropriate for an Abstractions project that should primarily contain interfaces, constants, and shared contracts without runtime dependencies.
1 participant
Closes #4
Summary by Sourcery
Introduce new Hosting and Abstractions projects with initial dependencies and configuration, including Orleans integration for runtime hosting and shared constants, and update the solution and build configurations accordingly
New Features:
Enhancements:
Chores:
Summary by CodeRabbit
New Features
Chores