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

Add option to generate docker compose file with enviroment variables instead of secrets #254

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Agrabski
Copy link

@Agrabski Agrabski commented Aug 22, 2024

User description

A very basic implementation on #151. Environment variable names are generated by capitalising parameter names and secret management is disabled while using the new flag.


PR Type

enhancement, documentation


Description

  • Added a new action PopulateInputsWithEnvVariablesAction to generate environment variable placeholders for inputs.
  • Introduced a new command option UseEnvVariablesAsParameterValues to replace parameter references with environment variable names.
  • Updated the command handler to conditionally execute actions based on the new option.
  • Enhanced documentation to include the new CLI option for generating Docker Compose files with environment variables.

Changes walkthrough 📝

Relevant files
Enhancement
11 files
PopulateInputsWithEnvVariablesAction.cs
Add PopulateInputsWithEnvVariablesAction for environment variables

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

  • Added a new action PopulateInputsWithEnvVariablesAction.
  • Generates environment variable placeholders for inputs.
  • Replaces parameter names with uppercase environment variable names.
  • +28/-0   
    BaseCommand.cs
    Conditional secret loading based on command options           

    src/Aspirate.Commands/Commands/BaseCommand.cs

    • Conditional loading of secrets based on RequiresSecrets flag.
    +4/-3     
    BaseCommandOptions.cs
    Add RequiresSecrets property to command options                   

    src/Aspirate.Commands/Commands/BaseCommandOptions.cs

    • Introduced RequiresSecrets property.
    +1/-0     
    GenerateCommand.cs
    Add UseEnvVariablesAsParameterValues option to GenerateCommand

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

    • Added new option UseEnvVariablesAsParameterValues.
    +23/-22 
    GenerateCommandHandler.cs
    Modify action sequence for environment variable support   

    src/Aspirate.Commands/Commands/Generate/GenerateCommandHandler.cs

  • Modified action sequence to support environment variable option.
  • Conditional execution of PopulateInputsWithEnvVariablesAction.
  • +24/-9   
    GenerateOptions.cs
    Add UseEnvVariablesAsParameterValues property to GenerateOptions

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

  • Added UseEnvVariablesAsParameterValues property.
  • Adjusted RequiresSecrets logic.
  • +3/-0     
    UseEnvVariablesAsParameterValuesOption.cs
    Create UseEnvVariablesAsParameterValuesOption class           

    src/Aspirate.Commands/Options/UseEnvVariablesAsParameterValuesOption.cs

    • Created new option class for environment variable usage.
    +16/-0   
    ServiceCollectionExtensions.cs
    Register PopulateInputsWithEnvVariablesAction in service collection

    src/Aspirate.Commands/ServiceCollectionExtensions.cs

    • Registered PopulateInputsWithEnvVariablesAction.
    +2/-1     
    JsonExpressionProcessor.cs
    Update regex for environment variable placeholders             

    src/Aspirate.Processors/Transformation/Json/JsonExpressionProcessor.cs

    • Updated regex to handle environment variable placeholders.
    +7/-2     
    IGenerateOptions.cs
    Extend IGenerateOptions with environment variable option 

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

    • Added UseEnvVariablesAsParameterValues to interface.
    +1/-0     
    AspirateState.cs
    Add UseEnvVariablesAsParameterValues to AspirateState       

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

    • Added UseEnvVariablesAsParameterValues property.
    +7/-4     
    Documentation
    1 files
    Generate-Command.md
    Document UseEnvVariablesAsParameterValues CLI option         

    docs/Writerside/topics/Generate-Command.md

    • Documented new CLI option for environment variables.
    +36/-28 

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    @codiumai-pr-agent-pro codiumai-pr-agent-pro bot added documentation Improvements or additions to documentation enhancement New feature or request Review effort [1-5]: 3 labels Aug 22, 2024
    Copy link

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Conditional Logic
    The new implementation introduces conditional logic based on the useEnvVariablesAsParameterValues flag. This might lead to different behavior paths that need to be thoroughly tested.

    Regex Change
    The regex pattern for placeholder detection has been modified. This change could potentially affect how placeholders are identified and processed throughout the application.

    State Management
    A new property UseEnvVariablesAsParameterValues has been added to the AspirateState class. Ensure that this new state is properly managed and doesn't introduce any inconsistencies in the application's state handling.

    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Best practice
    Use named capture groups in regex for improved readability

    Consider using a named capture group in the regex pattern for better readability and
    to avoid magic numbers when accessing groups.

    src/Aspirate.Processors/Transformation/Json/JsonExpressionProcessor.cs [29-30]

    -[GeneratedRegex(@"(\$|)\{([\w\.-]+)\}")]
    +[GeneratedRegex(@"(?<prefix>\$|)\{(?<content>[\w\.-]+)\}")]
     private static partial Regex PlaceholderPatternRegex();
     
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: Using named capture groups significantly improves code readability and maintainability by making the regex pattern more understandable and reducing reliance on magic numbers.

    9
    Enhancement
    Refactor nested if statements to use pattern matching for improved readability

    Consider using pattern matching with 'when' clause instead of nested if statements
    to improve readability and reduce nesting.

    src/Aspirate.Processors/Transformation/Json/JsonExpressionProcessor.cs [103-115]

     var matches = PlaceholderPatternRegex().Matches(input);
    -for (var i = 0; i < matches.Count; i++)
    +foreach (Match match in matches)
     {
    -    var match = matches[i];
    -    if (!string.IsNullOrEmpty(match.Groups[1].Value))
    +    var prefix = match.Groups["prefix"].Value;
    +    var content = match.Groups["content"].Value;
    +    
    +    if (string.IsNullOrEmpty(prefix))
         {
    -        continue;
    +        var pathParts = content.Split('.');
    +        input = pathParts.Length switch
    +        {
    +            1 => input.Replace($"{{{content}}}", rootNode[pathParts[0]].ToString(), StringComparison.OrdinalIgnoreCase),
    +            _ => input // Handle multi-part paths here
    +        };
         }
    +}
     
    -    var jsonPath = match.Groups[2].Value;
    -    var pathParts = jsonPath.Split('.');
    -    if (pathParts.Length == 1)
    -    {
    -        input = input.Replace($"{{{jsonPath}}}", rootNode[pathParts[0]].ToString(), StringComparison.OrdinalIgnoreCase);
    -
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The refactoring suggestion enhances readability by reducing nesting and using pattern matching, which is a modern and cleaner approach in C#. It improves the code structure without altering functionality.

    8
    Refactor the manifest generation logic to improve extensibility and maintainability

    Consider using a strategy pattern or factory method to create the appropriate action
    sequence based on the output format and whether environment variables are used for
    parameter values. This would simplify the HandleAsync method and make it easier to
    add new output formats in the future.

    src/Aspirate.Commands/Commands/Generate/GenerateCommandHandler.cs [12-17]

    -return outputFormat.Name switch
    +return GenerateManifests(outputFormat.Name, options.UseEnvVariablesAsParameterValues ?? false);
    +
    +// Add this method:
    +private Task<int> GenerateManifests(string outputFormat, bool useEnvVariablesAsParameterValues)
     {
    -    nameof(OutputFormat.Kustomize) => GenerateKustomizeManifests(),
    -    nameof(OutputFormat.DockerCompose) => GenerateDockerComposeManifests(options.UseEnvVariablesAsParameterValues ?? false),
    -    nameof(OutputFormat.Helm) => GenerateHelmManifests(),
    -    _ => throw new ArgumentOutOfRangeException(nameof(options.OutputFormat), $"The output format '{options.OutputFormat}' is not supported."),
    -};
    +    return outputFormat switch
    +    {
    +        nameof(OutputFormat.Kustomize) => GenerateKustomizeManifests(),
    +        nameof(OutputFormat.DockerCompose) => GenerateDockerComposeManifests(useEnvVariablesAsParameterValues),
    +        nameof(OutputFormat.Helm) => GenerateHelmManifests(),
    +        _ => throw new ArgumentOutOfRangeException(nameof(outputFormat), $"The output format '{outputFormat}' is not supported."),
    +    };
    +}
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion offers a refactoring that could improve the extensibility and maintainability of the code by centralizing the logic for generating manifests. However, it is not a critical change and mainly enhances code organization.

    7
    Maintainability
    Extract common action sequence to reduce duplication in the BaseGenerateActionSequence method

    Consider extracting the common parts of the BaseGenerateActionSequence method into a
    separate method to reduce code duplication and improve readability.

    src/Aspirate.Commands/Commands/Generate/GenerateCommandHandler.cs [21-48]

     private ActionExecutor BaseGenerateActionSequence(bool useEnvVariablesAsParameterValues = false)
     {
    -    var result = ActionExecutor
    +    var result = CommonActionSequence();
    +    
    +    if (useEnvVariablesAsParameterValues)
    +    {
    +        result.QueueAction(nameof(PopulateInputsWithEnvVariablesAction));
    +    }
    +    else
    +    {
    +        result.QueueAction(nameof(PopulateInputsAction))
    +              .QueueAction(nameof(SaveSecretsAction));
    +    }
    +
    +    return result;
    +}
    +
    +private ActionExecutor CommonActionSequence()
    +{
    +    return ActionExecutor
             .QueueAction(nameof(LoadConfigurationAction))
             .QueueAction(nameof(GenerateAspireManifestAction))
             .QueueAction(nameof(LoadAspireManifestAction))
    -        .QueueAction(nameof(IncludeAspireDashboardAction));
    -    if (!useEnvVariablesAsParameterValues)
    -    {
    -        result.QueueAction(nameof(PopulateInputsAction));
    -    }
    -    else
    -    {
    -        result.QueueAction(nameof(PopulateInputsWithEnvVariablesAction));
    -    }
    -    result
    +        .QueueAction(nameof(IncludeAspireDashboardAction))
             .QueueAction(nameof(SubstituteValuesAspireManifestAction))
             .QueueAction(nameof(ApplyDaprAnnotationsAction))
             .QueueAction(nameof(PopulateContainerDetailsForProjectsAction))
             .QueueAction(nameof(BuildAndPushContainersFromProjectsAction))
             .QueueAction(nameof(BuildAndPushContainersFromDockerfilesAction));
    -    if (!useEnvVariablesAsParameterValues)
    -    {
    -        result.QueueAction(nameof(SaveSecretsAction));
    -    }
    -
    -    return result;
     }
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: This suggestion effectively reduces code duplication and enhances readability, which is beneficial for maintainability. It is a good practice to extract common logic into separate methods.

    8

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    documentation Improvements or additions to documentation enhancement New feature or request Review effort [1-5]: 3
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    1 participant