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

Possibility to specify manifest parameter values for generate command in non interactive mode (#274) #277

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dombrovsky
Copy link

@dombrovsky dombrovsky commented Jan 2, 2025

User description

resolves #274
Allows to set parameters to generate command from command line: generate --parameter Key1=Value1 --parameter Key2=Value2.
Useful when used in non-interactive mode.


PR Type

Enhancement


Description

  • Added support for specifying manifest parameters in non-interactive mode.

  • Introduced new option --parameter for generate command.

  • Implemented logic to override parameter values programmatically.

  • Updated state and interfaces to handle parameter overrides.


Changes walkthrough 📝

Relevant files
Enhancement
PopulateInputsAction.cs
Implement parameter override handling in PopulateInputsAction

src/Aspirate.Commands/Actions/Secrets/PopulateInputsAction.cs

  • Added logic to handle parameter overrides.
  • Introduced ApplyOverriddenValues method for parameter assignment.
  • Adjusted existing methods to integrate overridden parameters.
  • +26/-2   
    GenerateCommand.cs
    Add parameter option to generate command                                 

    src/Aspirate.Commands/Commands/Generate/GenerateCommand.cs

  • Added ParameterResourceValueOption to the generate command options.
  • +1/-0     
    GenerateOptions.cs
    Extend GenerateOptions with Parameters property                   

    src/Aspirate.Commands/Commands/Generate/GenerateOptions.cs

    • Added Parameters property to handle parameter values.
    +1/-0     
    ParameterResourceValueOption.cs
    Add ParameterResourceValueOption for parameter input         

    src/Aspirate.Commands/Options/ParameterResourceValueOption.cs

  • Introduced ParameterResourceValueOption for parameter value input.
  • Defined aliases and description for the option.
  • +20/-0   
    IGenerateOptions.cs
    Update IGenerateOptions to include Parameters                       

    src/Aspirate.Shared/Interfaces/Commands/Contracts/IGenerateOptions.cs

    • Added Parameters property to the IGenerateOptions interface.
    +1/-0     
    AspirateState.cs
    Add Parameters property to AspirateState                                 

    src/Aspirate.Shared/Models/Aspirate/AspirateState.cs

  • Added Parameters property to the AspirateState class.
  • Marked Parameters as restorable state property.
  • +4/-0     

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

    Copy link

    qodo-merge-pro bot commented Jan 2, 2025

    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:

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

    Input Validation

    The parameter value splitting logic assumes input will always contain '=' character. Missing validation could lead to IndexOutOfRangeException if input format is invalid.

    var parametersOverride = CurrentState.Parameters?.Select(s =>
        {
            var parts = s.Split('=', 2);
            return new KeyValuePair<string, string>(parts[0], parts[1]);
        })
        .ToDictionary();
    Potential Bug

    The ToDictionary() call doesn't handle duplicate keys which could silently ignore subsequent parameter values for the same key.

    var parametersOverride = CurrentState.Parameters?.Select(s =>
        {
            var parts = s.Split('=', 2);
            return new KeyValuePair<string, string>(parts[0], parts[1]);
        })
        .ToDictionary();

    Copy link

    qodo-merge-pro bot commented Jan 2, 2025

    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                                                                                                                                    Score
    Possible issue
    Add input validation for parameter override strings to prevent runtime errors from malformed input

    Add input validation to ensure parameter override strings contain the required '='
    separator and have both key and value parts. Currently, the code assumes the format
    is correct which could lead to runtime errors.

    src/Aspirate.Commands/Actions/Secrets/PopulateInputsAction.cs [19-24]

    -var parametersOverride = CurrentState.Parameters?.Select(s =>
    -    {
    +var parametersOverride = CurrentState.Parameters?
    +    .Where(s => s.Contains('='))
    +    .Select(s => {
             var parts = s.Split('=', 2);
    -        return new KeyValuePair<string, string>(parts[0], parts[1]);
    +        if (parts.Length != 2 || string.IsNullOrEmpty(parts[0]) || string.IsNullOrEmpty(parts[1]))
    +            throw new ArgumentException($"Invalid parameter format: {s}. Expected format: key=value");
    +        return new KeyValuePair<string, string>(parts[0].Trim(), parts[1].Trim());
         })
         .ToDictionary();
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: The suggestion addresses a critical input validation issue that could cause runtime errors. Adding validation for the parameter format and proper error handling significantly improves the code's robustness and user experience.

    9
    Prevent null reference exceptions by providing a default empty dictionary when parameters are null

    Handle the case where CurrentState.Parameters is null by initializing
    parametersOverride to an empty dictionary, preventing potential
    NullReferenceException in subsequent operations.

    src/Aspirate.Commands/Actions/Secrets/PopulateInputsAction.cs [19-24]

     var parametersOverride = CurrentState.Parameters?.Select(s =>
         {
             var parts = s.Split('=', 2);
             return new KeyValuePair<string, string>(parts[0], parts[1]);
         })
    -    .ToDictionary();
    +    .ToDictionary() ?? new Dictionary<string, string>();
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: This suggestion fixes a potential null reference exception that could crash the application. Providing a default empty dictionary is a crucial defensive programming practice for this scenario.

    8

    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.

    Add ability to pass in manifest inputs into the "aspirate generate" command.
    1 participant