GitVersion + Heading Action Endpoint #15
Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... 📒 Files selected for processing (7)
Tip You can customize the tone of the review comments and chat replies.Set the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (6)
src/Argon.Api/GlobalVersion.cs (1)
1-4: Add XML documentation for better API documentation.Consider adding XML documentation to describe the purpose of this class and its role in version management.
namespace Argon.Api; +/// <summary> +/// Provides access to version information generated by GitVersion. +/// This class exposes version metadata that can be used across the application. +/// </summary> public static class GlobalVersionsrc/Argon.Api/Controllers/MetadataController.cs (3)
6-8: Consider adding API documentation and versioning.The controller would benefit from:
- XML documentation for OpenAPI/Swagger
- API versioning to facilitate future evolution
Add the following attributes:
[ApiController] +[ApiVersion("1.0")] +/// <summary> +/// Provides configuration and metadata about the Argon service. +/// </summary> public class MetadataController : ControllerBase
9-11: Consider using HTTP GET attribute explicitly.While the route will default to GET, it's better to be explicit about the HTTP method for clarity and documentation purposes.
[Route("/cfg.json")] +[HttpGet] [AllowAnonymous]
26-36: Add validation attributes and documentation to records.The DTOs would benefit from input validation and XML documentation for better API documentation.
+/// <summary> +/// Represents the head routing configuration for the Argon service. +/// </summary> public record HeadRoutingConfig( + [Required] string version, + [Required] + [Url] string masterEndpoint, + [Required] + [Url] string webRtcEndpoint, + [Required] + [MinLength(1)] List<RegionalNode> cdnAddresses, + [Required] List<FeatureFlag> features ); +/// <summary> +/// Represents a regional CDN node configuration. +/// </summary> public record RegionalNode( + [Required] + [Url] string url, + [Required] + [RegularExpression("^[a-z]{2}[0-9]$", ErrorMessage = "Code must be in format: xx0")] string code); +/// <summary> +/// Represents a feature flag configuration. +/// </summary> public record FeatureFlag( + [Required] string code, bool enabled);src/Argon.Api/Program.cs (1)
6-6: Remove redundant namespace import.The
using Argon.Apidirective is redundant as more specific namespaces from the same root (e.g.,Argon.Api.Entities,Argon.Api.Extensions) are already imported.-using Argon.Api;src/Argon.Api/Argon.Api.csproj (1)
53-55: Standardize package reference attributes.The GitVersion.MsBuild package addition is good, but let's align it with the project's package reference conventions:
- Use lowercase "all" for PrivateAssets
- Add IncludeAssets to specify which assets should be included during build
Apply this diff to standardize the attributes:
<PackageReference Include="GitVersion.MsBuild" Version="5.12"> - <PrivateAssets>All</PrivateAssets> + <PrivateAssets>all</PrivateAssets> + <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets> </PackageReference>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
Argon.Server.sln(1 hunks)Directory.Build.props(1 hunks)GitVersion.yml(1 hunks)src/Argon.Api/Argon.Api.csproj(1 hunks)src/Argon.Api/Controllers/MetadataController.cs(1 hunks)src/Argon.Api/GlobalVersion.cs(1 hunks)src/Argon.Api/Program.cs(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- Argon.Server.sln
🧰 Additional context used
🪛 yamllint
GitVersion.yml
[error] 8-8: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (9)
GitVersion.yml (3)
1-8: LGTM with minor suggestions above.
The overall GitVersion configuration is well-structured:
- Continuous Delivery mode is appropriate for most projects
- Assembly versioning formats are well-defined
- Comprehensive patch version bump message pattern
- Starting at version 0.1 correctly indicates pre-release status
🧰 Tools
🪛 yamllint
[error] 8-8: no new line character at the end of file
(new-line-at-end-of-file)
4-4: 🛠️ Refactor suggestion
Consider defining merge message formats.
The empty merge message formats configuration (merge-message-formats: {}) might limit GitVersion's ability to handle version bumping based on merge commits. Consider defining formats to automate version increments on merges.
Here's a suggested configuration:
-merge-message-formats: {}
+merge-message-formats:
+ feature: '{BranchName}'
+ hotfix: '{BranchName}'
+ release: '{BranchName}'7-7:
Review the tag prefix configuration.
Setting tag-prefix: 'IGNORE' is unusual and might prevent GitVersion from recognizing version tags. Unless there's a specific reason for this, consider using a more conventional prefix like 'v'.
Suggested change:
-tag-prefix: 'IGNORE'
+tag-prefix: 'v'✅ Verification successful
Setting tag-prefix: 'IGNORE' appears to be intentional for a new repository
Since this is a newly set up repository with GitVersion (as evidenced by the recent commit "add gitversion"), and there are no existing version tags yet, using IGNORE as the tag prefix won't affect any existing versioning. This could be a deliberate choice to prevent GitVersion from looking for version tags until the versioning strategy is fully established.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any existing version tags that would be affected
git tag | grep -E '^v?\d+\.\d+\.\d+'
Length of output: 38
Script:
#!/bin/bash
# Let's check all tags to understand the current tagging pattern
git tag
# Also check the git log for any commit messages related to versioning
git log --oneline | grep -i "version"
# Look for any documentation about versioning
fd README.md | xargs cat | grep -i "version"
Length of output: 115
src/Argon.Api/GlobalVersion.cs (1)
5-17: Verify GitVersion integration in the project.
Ensure that GitVersion is properly configured and the GitVersionInformation class is being generated.
✅ Verification successful
Let me generate one more verification script to check for the GitVersion package reference in the project file specifically for the Argon.Api project.
GitVersion is properly integrated and configured ✅
The verification shows that GitVersion is correctly set up:
- GitVersion.yml exists with proper configuration for continuous delivery
- GitVersion.MsBuild package is referenced in Argon.Api.csproj
- GitVersionInformation class is being used correctly in GlobalVersion.cs
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify GitVersion setup and configuration
# Test 1: Check if GitVersion.yml exists
echo "Checking for GitVersion.yml..."
if [ -f "GitVersion.yml" ]; then
echo "Found GitVersion.yml"
cat GitVersion.yml
else
echo "GitVersion.yml not found"
fi
# Test 2: Check if GitVersion is referenced in the project file
echo -e "\nChecking for GitVersion package reference..."
rg -l "GitVersion" --type xml
# Test 3: Check for GitVersionInformation usage
echo -e "\nChecking for GitVersionInformation usage..."
rg "GitVersionInformation\." --type cs
Length of output: 2415
Script:
#!/bin/bash
# Check for GitVersion package reference in Argon.Api project file
echo "Checking Argon.Api project file for GitVersion..."
fd "Argon.Api.csproj" --exec cat {}
Length of output: 2797
src/Argon.Api/Controllers/MetadataController.cs (1)
13-13: Verify GlobalVersion class implementation.
Ensure that the GlobalVersion class correctly exposes the required properties.
✅ Verification successful
GlobalVersion class implementation is correct and properly exposes required properties
The GlobalVersion class correctly exposes both FullSemVer and ShortSha properties as static string properties, which are being used in the MetadataController. These properties are backed by GitVersionInformation, which is a standard versioning information provider.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify GlobalVersion class implementation
ast-grep --pattern 'class GlobalVersion {
$$$
public static string FullSemVer { get; $$ }
$$$
public static string ShortSha { get; $$ }
$$$
}'
Length of output: 154
Script:
#!/bin/bash
# Let's try a broader search for GlobalVersion class
rg -l "class GlobalVersion"
# Search for GlobalVersion properties
rg "GlobalVersion\." -A 2 -B 2
# Search for any file containing GlobalVersion
fd GlobalVersion
Length of output: 916
Script:
#!/bin/bash
# Let's examine the content of GlobalVersion.cs
cat src/Argon.Api/GlobalVersion.cs
Length of output: 1054
Directory.Build.props (3)
22-24: LGTM! SourceLink configuration is properly set up
The SourceLink configuration is correct and follows best practices:
- Using the latest stable version
- Properly marked as a private asset
- Aligns with the enabled
EmbedUntrackedSourcessetting
9-21:
Review metadata configuration
A few observations about the metadata configuration:
CS9113is listed twice in theNoWarnlist- The large number of suppressed warnings (10) might hide important issues. Consider documenting why each warning is suppressed.
- Verify if 2024 is the correct starting year for the copyright notice.
Fix the duplicate warning code:
- <NoWarn>CS0162,CS8981,CS8632,CS9113,CS8618,CS9113,CS8321,CS0219,CS1998,IDE0053</NoWarn>
+ <NoWarn>CS0162,CS8981,CS8632,CS9113,CS8618,CS8321,CS0219,CS1998,IDE0053</NoWarn>Let's check the project history to verify the copyright year:
#!/bin/bash
# Get the earliest commit date
git log --reverse --format=%cd --date=format:%Y | head -n 12-8: Review build configuration settings
Several configuration choices need careful consideration:
- The hardcoded
Versionproperty might conflict with GitVersion. Consider removing it to allow GitVersion to manage versioning. - Enabling preview features globally (
EnablePreviewFeatures) could lead to stability issues in production. Consider enabling this only for specific projects that require it. - Consider moving
AllowUnsafeBlocksto specific projects that require unsafe code, rather than enabling it solution-wide. InvariantGlobalizationmight affect culture-specific operations. Ensure this aligns with the application's internationalization requirements.
Let's verify the GitVersion configuration:
src/Argon.Api/Argon.Api.csproj (1)
53-55: Verify GitVersion configuration.
Let's ensure the GitVersion integration is properly configured in Directory.Build.props.
✅ Verification successful
Let me check the content of the GitVersion.yml file to verify the configuration.
GitVersion configuration is properly set up
The GitVersion configuration is correctly established with:
- A valid GitVersion.yml file in the root with proper configuration for continuous delivery
- Correct package reference in Argon.Api.csproj with PrivateAssets set
- No redundant or conflicting configurations in other project files
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify GitVersion configuration in Directory.Build.props
# Expected: Directory.Build.props should contain GitVersion configuration
# Check if Directory.Build.props exists and contains GitVersion settings
rg -l "GitVersion" Directory.Build.props
# Check for any existing GitVersion configuration files
fd -g "GitVersion*.yml"
Length of output: 81
Script:
#!/bin/bash
# Check the content of GitVersion.yml
cat GitVersion.yml
# Also check if there are any other GitVersion related configurations in the project
rg -g "*.props" -g "*.csproj" "GitVersion" -A 3
Length of output: 788
| commit-message-incrementing: Enabled | ||
| patch-version-bump-message: "^(add|bump|drop|mv|move|update|expose|remove|refactoring|additional|use|improvement|fix|refactor)(.+)?" | ||
| tag-prefix: 'IGNORE' | ||
| next-version: "0.1" |
There was a problem hiding this comment.
Add newline at end of file.
The file is missing a newline character at the end, which is a POSIX requirement and can cause issues with some tools.
Add a newline after the last line:
next-version: "0.1"
+📝 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.
| next-version: "0.1" | |
| next-version: "0.1" | |
🧰 Tools
🪛 yamllint
[error] 8-8: no new line character at the end of file
(new-line-at-end-of-file)
| public static string Major => GitVersionInformation.Major; | ||
| public static string Minor => GitVersionInformation.Minor; | ||
| public static string Patch => GitVersionInformation.Patch; |
There was a problem hiding this comment.
Convert version components to numeric types.
Major, Minor, and Patch should be numeric types to enable proper version comparison and arithmetic operations.
- public static string Major => GitVersionInformation.Major;
- public static string Minor => GitVersionInformation.Minor;
- public static string Patch => GitVersionInformation.Patch;
+ public static int Major => int.Parse(GitVersionInformation.Major);
+ public static int Minor => int.Parse(GitVersionInformation.Minor);
+ public static int Patch => int.Parse(GitVersionInformation.Patch);| public static string BranchName => GitVersionInformation.BranchName; | ||
| public static string AssemblySemFileVer => GitVersionInformation.AssemblySemFileVer; | ||
| public static string AssemblySemVer => GitVersionInformation.AssemblySemVer; | ||
| public static string CommitDate => GitVersionInformation.CommitDate; | ||
| public static string BuildMetaData => GitVersionInformation.BuildMetaData; | ||
| public static string FullSemVer => GitVersionInformation.FullSemVer; | ||
| public static string InformationalVersion => GitVersionInformation.InformationalVersion; | ||
| public static string Major => GitVersionInformation.Major; | ||
| public static string Minor => GitVersionInformation.Minor; | ||
| public static string Patch => GitVersionInformation.Patch; | ||
| public static string Sha => GitVersionInformation.Sha; | ||
| public static string ShortSha => GitVersionInformation.ShortSha; | ||
| public static string PreReleaseTag => GitVersionInformation.PreReleaseTag; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and reorganize properties.
Consider these improvements:
- Add error handling for GitVersionInformation initialization
- Group related properties together
- Add computed properties for common version formats
public static class GlobalVersion
{
+ private static void EnsureInitialized()
+ {
+ if (GitVersionInformation.BranchName == null)
+ throw new InvalidOperationException("GitVersion information is not properly initialized.");
+ }
+
+ // Core version components
+ public static int Major => int.Parse(GitVersionInformation.Major);
+ public static int Minor => int.Parse(GitVersionInformation.Minor);
+ public static int Patch => int.Parse(GitVersionInformation.Patch);
+ public static string PreReleaseTag => GitVersionInformation.PreReleaseTag;
+
+ // Git information
+ public static string BranchName => GitVersionInformation.BranchName;
+ public static string Sha => GitVersionInformation.Sha;
+ public static string ShortSha => GitVersionInformation.ShortSha;
+ public static string CommitDate => GitVersionInformation.CommitDate;
+
+ // Build metadata
+ public static string BuildMetaData => GitVersionInformation.BuildMetaData;
+
+ // Formatted versions
+ public static string AssemblySemFileVer => GitVersionInformation.AssemblySemFileVer;
+ public static string AssemblySemVer => GitVersionInformation.AssemblySemVer;
+ public static string FullSemVer => GitVersionInformation.FullSemVer;
+ public static string InformationalVersion => GitVersionInformation.InformationalVersion;
+
+ // Computed properties
+ public static string SimpleVersion => $"{Major}.{Minor}.{Patch}";
+ public static string VersionWithHash => $"{FullSemVer}+{ShortSha}";Committable suggestion skipped: line range outside the PR's diff.
| public ValueTask<HeadRoutingConfig> GetHead() => | ||
| new(new HeadRoutingConfig( | ||
| $"{GlobalVersion.FullSemVer}.{GlobalVersion.ShortSha}", | ||
| "api.argon.gl", | ||
| "argon-f14ic5ia.livekit.cloud", | ||
| [ | ||
| new RegionalNode("cdn-ru1.argon.gl", "ru1"), | ||
| new RegionalNode("cdn-ru2.argon.gl", "ru1"), | ||
| new RegionalNode("cdn-as1.argon.gl", "as1") | ||
| ], [ | ||
| new FeatureFlag("dev.window", true), | ||
| new FeatureFlag("user.allowServerCreation", true), | ||
| ])); |
There was a problem hiding this comment.
Move hardcoded configuration to appsettings.json.
The current implementation hardcodes several configuration values that should be externalized for better maintainability and environment-specific configuration.
- Create a configuration class:
public class ArgonConfig
{
public string MasterEndpoint { get; set; }
public string WebRtcEndpoint { get; set; }
public List<RegionalNode> CdnNodes { get; set; }
public List<FeatureFlag> Features { get; set; }
}- Inject and use configuration:
public class MetadataController : ControllerBase
{
+ private readonly ArgonConfig _config;
+
+ public MetadataController(IOptions<ArgonConfig> config)
+ {
+ _config = config.Value;
+ }
+
[Route("/cfg.json")]
[AllowAnonymous]
- public ValueTask<HeadRoutingConfig> GetHead() =>
- new(new HeadRoutingConfig(
+ public HeadRoutingConfig GetHead() =>
+ new HeadRoutingConfig(
$"{GlobalVersion.FullSemVer}.{GlobalVersion.ShortSha}",
- "api.argon.gl",
- "argon-f14ic5ia.livekit.cloud",
- [
- new RegionalNode("cdn-ru1.argon.gl", "ru1"),
- new RegionalNode("cdn-ru2.argon.gl", "ru1"),
- new RegionalNode("cdn-as1.argon.gl", "as1")
- ], [
- new FeatureFlag("dev.window", true),
- new FeatureFlag("user.allowServerCreation", true),
- ]));
+ _config.MasterEndpoint,
+ _config.WebRtcEndpoint,
+ _config.CdnNodes,
+ _config.Features);| new RegionalNode("cdn-ru1.argon.gl", "ru1"), | ||
| new RegionalNode("cdn-ru2.argon.gl", "ru1"), | ||
| new RegionalNode("cdn-as1.argon.gl", "as1") |
There was a problem hiding this comment.
Fix duplicate region code.
There appears to be a typo in the region codes where two nodes are assigned to "ru1".
[
new RegionalNode("cdn-ru1.argon.gl", "ru1"),
- new RegionalNode("cdn-ru2.argon.gl", "ru1"),
+ new RegionalNode("cdn-ru2.argon.gl", "ru2"),
new RegionalNode("cdn-as1.argon.gl", "as1")📝 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.
| new RegionalNode("cdn-ru1.argon.gl", "ru1"), | |
| new RegionalNode("cdn-ru2.argon.gl", "ru1"), | |
| new RegionalNode("cdn-as1.argon.gl", "as1") | |
| new RegionalNode("cdn-ru1.argon.gl", "ru1"), | |
| new RegionalNode("cdn-ru2.argon.gl", "ru2"), | |
| new RegionalNode("cdn-as1.argon.gl", "as1") |
| @@ -42,6 +43,5 @@ | |||
| app.MapDefaultEndpoints(); | |||
| app.UseWebSockets(); | |||
| app.MapRpcWebSocketServer(); | |||
| var buildTime = File.GetLastWriteTimeUtc(typeof(Program).Assembly.Location); | |||
| app.MapGet("/", () => new { buildTime }); | |||
| app.MapGet("/", () => new { version = $"{GlobalVersion.FullSemVer}.{GlobalVersion.ShortSha}", }); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving version endpoint implementation.
The current implementation has several potential issues:
- No error handling if version properties are null/unavailable
- Direct dependency on static
GlobalVersionmakes unit testing difficult - Anonymous type response makes it harder for API consumers to understand the contract
Consider these improvements:
-app.MapGet("/", () => new { version = $"{GlobalVersion.FullSemVer}.{GlobalVersion.ShortSha}", });
+app.MapGet("/", () =>
+{
+ try {
+ var version = new VersionInfo(
+ fullVersion: GlobalVersion.FullSemVer,
+ commitSha: GlobalVersion.ShortSha
+ );
+ return Results.Ok(version);
+ }
+ catch (Exception ex)
+ {
+ return Results.StatusCode(500);
+ }
+});Add this record to define the response contract:
public record VersionInfo(string FullVersion, string CommitSha);Consider moving this to a dedicated metadata/health endpoint (e.g., /api/metadata) that could include other system information. This would better align with API best practices and allow for future expansion of system metadata.
Summary by CodeRabbit
Release Notes
New Features
/cfg.json) that provides configuration details including versioning and endpoint URLs.Improvements
GitVersion.ymlfor better version management.Directory.Build.propsfor improved configuration and metadata management.Bug Fixes
These updates enhance the application's versioning capabilities and improve the overall API response structure.