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

enha: allow custom env when local #1602

Merged
merged 1 commit into from
Aug 6, 2024
Merged

Conversation

gabriel-aranha-cw
Copy link
Contributor

@gabriel-aranha-cw gabriel-aranha-cw commented Aug 6, 2024

PR Type

Enhancement, Configuration changes


Description

  • Added support for custom local environment paths in the dotenv loading process.
  • Introduced a new environment configuration file for stratus-follower.
  • Updated justfile to remove hardcoded external RPC URLs and use LOCAL_ENV_PATH for stratus-follower.

Changes walkthrough 📝

Relevant files
Enhancement
config.rs
Support custom local environment paths in dotenv loading 

src/config.rs

  • Added support for custom local environment paths.
  • Modified logic to determine the .env file to load based on the
    environment.
  • Introduced handling for LOCAL_ENV_PATH environment variable.
  • +11/-4   
    Configuration changes
    stratus-follower.env.local
    Add environment configuration for stratus-follower             

    config/stratus-follower.env.local

  • Added new environment configuration file for stratus-follower.
  • Included various environment variables like RUST_LOG, CHAIN_ID, and
    EXTERNAL_RPC.
  • +10/-0   
    justfile
    Update justfile to use custom local environment paths       

    justfile

  • Removed hardcoded external RPC URLs.
  • Updated stratus-follower command to use LOCAL_ENV_PATH for environment
    configuration.
  • +2/-4     

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

    Copy link

    github-actions bot commented Aug 6, 2024

    PR Reviewer Guide 🔍

    (Review updated until commit 1ae2d97)

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

    Logging Improvement
    The initialization of the environment configuration should be logged with all relevant configurations to ensure proper tracking and debugging.

    Error Handling
    When logging errors, ensure the original error is logged in a field called reason for better traceability.

    @gabriel-aranha-cw gabriel-aranha-cw marked this pull request as ready for review August 6, 2024 13:00
    Copy link

    github-actions bot commented Aug 6, 2024

    Persistent review updated to latest commit 1ae2d97

    Copy link

    github-actions bot commented Aug 6, 2024

    PR Code Suggestions ✨

    Latest suggestions up to 1ae2d97

    CategorySuggestion                                                                                                                                    Score
    Security
    Use environment variables for sensitive information to improve security

    To improve security, consider using environment variables for sensitive information
    such as EXTERNAL_RPC and EXTERNAL_RPC_WS URLs instead of hardcoding them in the .env
    file.

    config/stratus-follower.env.local [9-10]

    -EXTERNAL_RPC=http://spec.testnet.cloudwalk.network:9934/
    -EXTERNAL_RPC_WS=ws://spec.testnet.cloudwalk.network:9946/
    +EXTERNAL_RPC=${EXTERNAL_RPC}
    +EXTERNAL_RPC_WS=${EXTERNAL_RPC_WS}
     
    Suggestion importance[1-10]: 10

    Why: Using environment variables for sensitive information is a best practice for security, reducing the risk of exposing sensitive data in version control.

    10
    Best practice
    Add a default value for LOCAL_ENV_PATH to ensure it is always set correctly

    To ensure that the LOCAL_ENV_PATH is always set correctly, consider adding a default
    value or a check to ensure it is not empty before using it in the stratus-follower
    command.

    justfile [94]

    -LOCAL_ENV_PATH=stratus-follower cargo {{nightly_flag}} run --bin stratus {{release_flag}} --features dev -- --follower {{args}}
    +LOCAL_ENV_PATH=${{LOCAL_ENV_PATH:-stratus-follower}} cargo {{nightly_flag}} run --bin stratus {{release_flag}} --features dev -- --follower {{args}}
     
    Suggestion importance[1-10]: 9

    Why: Ensuring LOCAL_ENV_PATH has a default value prevents potential issues with unset or empty environment variables, enhancing the robustness of the script.

    9
    Possible issue
    Use unwrap_or_else to handle potential errors when calling build_info::binary_name()

    To avoid potential issues with unhandled errors, consider using the unwrap_or_else
    method instead of unwrap when calling build_info::binary_name(). This will allow you
    to handle any errors gracefully.

    src/config.rs [45]

    -Err(_) => format!("config/{}.env.local", build_info::binary_name()),
    +Err(_) => format!("config/{}.env.local", build_info::binary_name().unwrap_or_else(|_| "default_binary_name")),
     
    Suggestion importance[1-10]: 8

    Why: This suggestion improves error handling by avoiding potential panics, which is crucial for robust code. However, the specific error handling logic might need further refinement based on the context.

    8
    Maintainability
    Extract the logic for determining the env_filename into a separate function for better readability

    To improve readability and maintainability, consider extracting the logic for
    determining the env_filename into a separate function. This will make the
    load_dotenv_file function cleaner and easier to understand.

    src/config.rs [40-53]

    -let env_filename = match env {
    -    Ok(Environment::Local) => {
    -        match std::env::var("LOCAL_ENV_PATH") {
    -            Ok(local_path) => format!("config/{}.env.local", local_path),
    -            Err(_) => format!("config/{}.env.local", build_info::binary_name()),
    +let env_filename = determine_env_filename(env);
    +
    +fn determine_env_filename(env: Result<Environment, _>) -> String {
    +    match env {
    +        Ok(Environment::Local) => {
    +            match std::env::var("LOCAL_ENV_PATH") {
    +                Ok(local_path) => format!("config/{}.env.local", local_path),
    +                Err(_) => format!("config/{}.env.local", build_info::binary_name()),
    +            }
    +        }
    +        Ok(env) => format!("config/{}.env.{}", build_info::binary_name(), env),
    +        Err(e) => {
    +            println!("{e}");
    +            return String::new();
             }
         }
    -    Ok(env) => format!("config/{}.env.{}", build_info::binary_name(), env),
    -    Err(e) => {
    -        println!("{e}");
    -        return;
    -    }
    -};
    +}
     
    Suggestion importance[1-10]: 7

    Why: Extracting the logic into a separate function enhances readability and maintainability, making the code cleaner and easier to understand. This is a good practice for complex logic.

    7

    Previous suggestions

    Suggestions up to commit 1ae2d97
    CategorySuggestion                                                                                                                                    Score
    Security
    Use HTTPS instead of HTTP for external RPC URLs to enhance security

    To enhance security, consider using HTTPS instead of HTTP for the EXTERNAL_RPC and
    EXTERNAL_RPC_WS URLs.

    config/stratus-follower.env.local [9-10]

    -EXTERNAL_RPC=http://spec.testnet.cloudwalk.network:9934/
    -EXTERNAL_RPC_WS=ws://spec.testnet.cloudwalk.network:9946/
    +EXTERNAL_RPC=https://spec.testnet.cloudwalk.network:9934/
    +EXTERNAL_RPC_WS=wss://spec.testnet.cloudwalk.network:9946/
     
    Suggestion importance[1-10]: 9

    Why: Using HTTPS instead of HTTP for external RPC URLs enhances security by encrypting the data transmitted between the client and server, which is a crucial improvement.

    9
    Maintainability
    Extract the logic for determining the environment filename into a separate function

    To improve readability and maintainability, consider extracting the logic for
    determining the env_filename into a separate function.

    src/config.rs [40-53]

    -let env_filename = match env {
    -    Ok(Environment::Local) => {
    -        match std::env::var("LOCAL_ENV_PATH") {
    -            Ok(local_path) => format!("config/{}.env.local", local_path),
    -            Err(_) => format!("config/{}.env.local", build_info::binary_name()),
    +let env_filename = determine_env_filename(env);
    +
    +fn determine_env_filename(env: Result<Environment, _>) -> String {
    +    match env {
    +        Ok(Environment::Local) => {
    +            match std::env::var("LOCAL_ENV_PATH") {
    +                Ok(local_path) => format!("config/{}.env.local", local_path),
    +                Err(_) => format!("config/{}.env.local", build_info::binary_name()),
    +            }
    +        }
    +        Ok(env) => format!("config/{}.env.{}", build_info::binary_name(), env),
    +        Err(e) => {
    +            println!("{e}");
    +            return String::new();
             }
         }
    -    Ok(env) => format!("config/{}.env.{}", build_info::binary_name(), env),
    -    Err(e) => {
    -        println!("{e}");
    -        return;
    -    }
    -};
    +}
     
    Suggestion importance[1-10]: 8

    Why: Extracting the logic for determining the env_filename into a separate function improves readability and maintainability by encapsulating the logic and making the main function cleaner.

    8
    Best practice
    Add a default value for the LOCAL_ENV_PATH variable to ensure it is always set correctly

    To ensure that the LOCAL_ENV_PATH variable is always set correctly, consider adding
    a default value or a check to confirm its presence before using it.

    justfile [94]

    -LOCAL_ENV_PATH=stratus-follower cargo {{nightly_flag}} run --bin stratus {{release_flag}} --features dev -- --follower {{args}}
    +LOCAL_ENV_PATH=${LOCAL_ENV_PATH:-stratus-follower} cargo {{nightly_flag}} run --bin stratus {{release_flag}} --features dev -- --follower {{args}}
     
    Suggestion importance[1-10]: 7

    Why: Adding a default value for LOCAL_ENV_PATH ensures that the variable is always set, which can prevent potential runtime errors if the variable is not defined.

    7

    @gabriel-aranha-cw gabriel-aranha-cw merged commit 6d55242 into main Aug 6, 2024
    34 checks passed
    @gabriel-aranha-cw gabriel-aranha-cw deleted the custom-local-env branch August 6, 2024 13:02
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    None yet
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    1 participant