Skip to content

Fix null deref issue in apply command#324

Open
John-Leitch wants to merge 1 commit intoprom3theu5:mainfrom
John-Leitch:apply-null-path-fix
Open

Fix null deref issue in apply command#324
John-Leitch wants to merge 1 commit intoprom3theu5:mainfrom
John-Leitch:apply-null-path-fix

Conversation

@John-Leitch
Copy link
Contributor

@John-Leitch John-Leitch commented Apr 28, 2025

User description

This PR covers #231. In cases where WriteSecretsOutToTempFiles is called, AspirateState.OutputPath is only set when read from state. In the event of ignoring previous state, this causes a NullReferenceException. This fix updates WriteSecretsOutToTempFiles to fall back to AspirateState.InputPath in such cases.


PR Type

Bug fix


Description

  • Fixes null dereference in apply command when previous state is ignored

  • Ensures fallback to InputPath if OutputPath is null during secret writing

  • Improves error handling for missing manifest path


Changes walkthrough 📝

Relevant files
Bug fix
KustomizeService.cs
Add fallback for manifest path to prevent null dereference

src/Aspirate.Services/Implementations/KustomizeService.cs

  • Adds fallback to InputPath if OutputPath is null when writing secrets
  • Throws explicit error if neither path is set, improving error
    messaging
  • Prevents null reference exception during manifest application
  • +4/-1     

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • @qodo-code-review
    Copy link

    Qodo Merge was enabled for this repository. To continue using it, please link your Git account with your Qodo account here.

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ✅

    231 - PR Code Verified

    Compliant requirements:

    • Fix the error "Value cannot be null. (Parameter 'path1')" when applying manifests to a Kubernetes cluster

    Requires further human verification:

    • Ensure the apply command works correctly with different Kubernetes clusters (docker-desktop, minikube, AKS)
    • Verify that the regression is fully fixed and the functionality works as it did before

    ⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Error Handling

    The new exception message is generic. Consider providing more context about which operation failed and what the user should do to resolve it.

    var manifestPath = (state.OutputPath ?? state.InputPath) ??
        throw new InvalidOperationException("Could not write secret, manifest path not set.");

    @qodo-code-review
    Copy link

    Qodo Merge was enabled for this repository. To continue using it, please link your Git account with your Qodo account here.

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    General
    Simplify redundant null check

    The current code uses a null-coalescing operator on the result of another
    null-coalescing operation, which is redundant. Simplify the expression by using
    a single null-coalescing chain with the exception at the end.

    src/Aspirate.Services/Implementations/KustomizeService.cs [57-58]

    -var manifestPath = (state.OutputPath ?? state.InputPath) ??
    +var manifestPath = state.OutputPath ?? state.InputPath ??
         throw new InvalidOperationException("Could not write secret, manifest path not set.");
    • Apply this suggestion
    Suggestion importance[1-10]: 2

    __

    Why: The suggestion correctly points out that the parentheses in (state.OutputPath ?? state.InputPath) are redundant due to the left-associativity of the null-coalescing operator. However, this is a minor stylistic improvement with negligible impact on readability or performance.

    Low
    • More

    @gregpakes
    Copy link

    Any chance this can be merged?

    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.

    2 participants