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.

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

feat: .net9 support #27

Merged
merged 1 commit into from
Nov 14, 2024
Merged

feat: .net9 support #27

merged 1 commit into from
Nov 14, 2024

Conversation

prom3theu5
Copy link
Member

@prom3theu5 prom3theu5 commented Nov 14, 2024

PR Type

enhancement, dependencies


Description

  • Introduced a new HealthCheckAnnotation class with a factory method for creating health checks.
  • Updated logger configuration to support SeqApiKey and improved configuration retrieval using GetValue.
  • Added .NET 9.0 support by updating target frameworks and adjusting package versions.
  • Removed and updated global using directives to streamline imports.

Changes walkthrough 📝

Relevant files
Enhancement
HealthCheckAnnotation.cs
Introduce HealthCheckAnnotation class with factory method

src/SimCube.Aspire.Hosting/Features/HealthChecks/HealthCheckAnnotation.cs

  • Added a new class HealthCheckAnnotation.
  • Implemented a factory method to create health checks.
  • Introduced a static Create method for resource connection strings.
  • +25/-0   
    HealthCheckExtensions.cs
    Add using directive for HealthCheckAnnotation                       

    src/SimCube.Aspire.Hosting/Features/Postgres/HealthCheckExtensions.cs

    • Added using directive for HealthCheckAnnotation.
    +2/-0     
    GlobalUsings.cs
    Update global using directives for Aspire features             

    src/SimCube.Aspire.Hosting/GlobalUsings.cs

  • Removed several global using directives.
  • Updated global using directives for Aspire features.
  • +1/-22   
    OtlpServiceExtensions.cs
    Enhance logger configuration with Seq API key support       

    src/SimCube.Aspire/Features/Otlp/OtlpServiceExtensions.cs

  • Updated logger configuration to use GetValue for configuration.
  • Added handling for SeqApiKey in Seq configuration.
  • +11/-7   
    SeqLiterals.cs
    Add SeqApiKey constant for configuration                                 

    src/SimCube.Aspire/Features/Seq/SeqLiterals.cs

    • Added SeqApiKey constant.
    +1/-0     
    SimCube.Aspire.Hosting.csproj
    Add .NET 9.0 to target frameworks                                               

    src/SimCube.Aspire.Hosting/SimCube.Aspire.Hosting.csproj

  • Changed target frameworks to include .NET 9.0.
  • Adjusted package references for compatibility.
  • +2/-3     
    SimCube.Aspire.csproj
    Update target frameworks and package references                   

    src/SimCube.Aspire/SimCube.Aspire.csproj

  • Changed target frameworks to include .NET 9.0.
  • Removed unnecessary package reference.
  • +1/-2     
    Dependencies
    Directory.Packages.props
    Update package versions for .NET 9.0 support                         

    Directory.Packages.props

  • Updated package versions for .NET 9.0 support.
  • Added conditional package versions for different target frameworks.
  • +41/-31 

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Copy link

    CI Failure Feedback 🧐

    Action: pull_request

    Failed stage: Run restore [❌]

    Failure summary:

    The action failed due to the following reasons:

  • The .NET SDK version being used (8.0.403) does not support targeting .NET 9.0.
  • The projects SimCube.Aspire.Hosting.csproj and SimCube.Aspire.csproj are set to target .NET 9.0,
    which is not supported by the current SDK.
  • To resolve this issue, either target .NET 8.0 or lower, or use a version of the .NET SDK that
    supports .NET 9.0.

  • Relevant error logs:
    1:  ##[group]Operating System
    2:  Ubuntu
    ...
    
    134:  shell: /usr/bin/bash -e {0}
    135:  ##[endgroup]
    136:  2024-11-14 18:06:51 - Cleaning directories...
    137:  ##[group]Run eng/ci.sh restore
    138:  �[36;1meng/ci.sh restore�[0m
    139:  shell: /usr/bin/bash -e {0}
    140:  ##[endgroup]
    141:  2024-11-14 18:06:51 - Restoring NuGet packages...
    142:  ##[error]/usr/share/dotnet/sdk/8.0.403/Sdks/Microsoft.NET.Sdk/targets/Microsoft.NET.TargetFrameworkInference.targets(166,5): error NETSDK1045: The current .NET SDK does not support targeting .NET 9.0.  Either target .NET 8.0 or lower, or use a version of the .NET SDK that supports .NET 9.0. Download the .NET SDK from https://aka.ms/dotnet/download [/home/runner/work/simcube-aspire/simcube-aspire/src/SimCube.Aspire.Hosting/SimCube.Aspire.Hosting.csproj::TargetFramework=net9.0]
    143:  ##[error]/usr/share/dotnet/sdk/8.0.403/Sdks/Microsoft.NET.Sdk/targets/Microsoft.NET.TargetFrameworkInference.targets(166,5): error NETSDK1045: The current .NET SDK does not support targeting .NET 9.0.  Either target .NET 8.0 or lower, or use a version of the .NET SDK that supports .NET 9.0. Download the .NET SDK from https://aka.ms/dotnet/download [/home/runner/work/simcube-aspire/simcube-aspire/src/SimCube.Aspire/SimCube.Aspire.csproj::TargetFramework=net9.0]
    144:  ##[error]Process completed with exit code 1.
    

    ✨ CI feedback usage guide:

    The CI feedback tool (/checks) automatically triggers when a PR has a failed check.
    The tool analyzes the failed checks and provides several feedbacks:

    • Failed stage
    • Failed test name
    • Failure summary
    • Relevant error logs

    In addition to being automatically triggered, the tool can also be invoked manually by commenting on a PR:

    /checks "https://github.com/{repo_name}/actions/runs/{run_number}/job/{job_number}"
    

    where {repo_name} is the name of the repository, {run_number} is the run number of the failed check, and {job_number} is the job number of the failed check.

    Configuration options

    • enable_auto_checks_feedback - if set to true, the tool will automatically provide feedback when a check is failed. Default is true.
    • excluded_checks_list - a list of checks to exclude from the feedback, for example: ["check1", "check2"]. Default is an empty list.
    • enable_help_text - if set to true, the tool will provide a help message with the feedback. Default is true.
    • persistent_comment - if set to true, the tool will overwrite a previous checks comment with the new feedback. Default is true.
    • final_update_message - if persistent_comment is true and updating a previous checks message, the tool will also create a new message: "Persistent checks updated to latest commit". Default is true.

    See more information about the checks tool in the docs.

    Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 Security concerns

    Sensitive information exposure:
    The SeqApiKey is being used in configuration but there are no safeguards to prevent it from being logged or exposed in error messages. Consider adding additional security measures around the handling of this API key.

    ⚡ Recommended focus areas for review

    Potential Bug
    The Create method XML documentation is incomplete and missing parameter description. This could lead to incorrect usage.

    Error Handling
    The removal of DbUpdateExceptionDestructurer could lead to less detailed error logging for database exceptions

    Configuration Validation
    No validation is performed on the SeqEndpoint and SeqApiKey configuration values before using them

    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Add parameter validation to prevent null reference exceptions

    Add null check for the healthCheckFactory parameter in the constructor to prevent
    potential null reference exceptions.

    src/SimCube.Aspire.Hosting/Features/HealthChecks/HealthCheckAnnotation.cs [3]

     public class HealthCheckAnnotation(Func<IResource, CancellationToken, Task<IHealthCheck?>> healthCheckFactory) : IResourceAnnotation
    +{
    +    ArgumentNullException.ThrowIfNull(healthCheckFactory);
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Adding null validation for constructor parameters is crucial for preventing runtime errors. This is especially important for a factory function that will be used throughout the application's lifecycle.

    8
    Add null check for method parameter to prevent null reference exceptions

    Add null check for the request parameter in the FilterHttpRequestMessage lambda to
    prevent potential null reference exceptions.

    src/SimCube.Aspire/Features/Otlp/OtlpServiceExtensions.cs [117-119]

     .AddHttpClientInstrumentation(options => options.FilterHttpRequestMessage = (HttpRequestMessage request) =>
    -    !request.RequestUri?.AbsoluteUri.Contains(builder.Configuration[OtlpLiterals.Endpoint],
    +    request == null || !request.RequestUri?.AbsoluteUri.Contains(builder.Configuration[OtlpLiterals.Endpoint],
             StringComparison.Ordinal) ?? true);
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Adding a null check for the request parameter in the lambda expression prevents potential NullReferenceException in the HTTP client instrumentation filter, improving the robustness of the telemetry system.

    7
    Performance
    Cache configuration value to improve performance by reducing redundant lookups

    Cache the configuration value for OtlpLiterals.ServiceName to avoid multiple lookups
    in the same method.

    src/SimCube.Aspire/Features/Otlp/OtlpServiceExtensions.cs [41-42]

    -.Enrich.WithProperty(nameof(OtlpLiterals.ServiceName), configuration[OtlpLiterals.ServiceName])
    +var serviceName = configuration[OtlpLiterals.ServiceName];
    +.Enrich.WithProperty(nameof(OtlpLiterals.ServiceName), serviceName)
     ...
    -options.ResourceAttributes.Add("service.name", configuration[OtlpLiterals.ServiceName]);
    +options.ResourceAttributes.Add("service.name", serviceName);
    Suggestion importance[1-10]: 4

    Why: While caching the configuration value would reduce redundant lookups, the performance impact is minimal as the method is likely called infrequently during application setup.

    4

    💡 Need additional feedback ? start a PR chat

    @prom3theu5 prom3theu5 merged commit 687e717 into main Nov 14, 2024
    2 checks passed
    @prom3theu5 prom3theu5 deleted the net9 branch November 14, 2024 18:10
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    1 participant