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: simplify defition of env-var aliases #1579

Merged
merged 2 commits into from
Jul 31, 2024
Merged

feat: simplify defition of env-var aliases #1579

merged 2 commits into from
Jul 31, 2024

Conversation

dinhani-cw
Copy link
Contributor

@dinhani-cw dinhani-cw commented Jul 31, 2024

PR Type

Enhancement


Description

  • Updated various configuration fields to include the executor_ prefix for clarity.
  • Added backward-compatible aliases for old environment variable names.
  • Modified EVM and executor configurations to use new field names.
  • Updated test configurations to reflect changes in executor field names.

Changes walkthrough 📝

Relevant files
Enhancement
run_with_importer.rs
Update RPC server initialization with new executor chain ID

src/bin/run_with_importer.rs

  • Updated chain_id to executor_chain_id in RPC server initialization.
  • +1/-1     
    evm.rs
    Update EVM configuration and transaction handling               

    src/eth/executor/evm.rs

  • Changed chain_id to executor_chain_id in EVM configuration.
  • Updated reject_not_contract to executor_reject_not_contract in
    transaction handling.
  • +2/-2     
    executor.rs
    Update executor strategy and EVM spawning configuration   

    src/eth/executor/executor.rs

  • Replaced strategy with executor_strategy in transaction execution.
  • Updated num_evms to executor_evms in EVM spawning.
  • +5/-5     
    executor_config.rs
    Rename executor configuration fields and add aliases         

    src/eth/executor/executor_config.rs

  • Renamed several configuration fields to include executor_ prefix.
  • Added aliases for backward compatibility with old environment variable
    names.
  • +14/-9   
    globals.rs
    Add and apply environment variable aliases                             

    src/globals.rs

  • Added env_alias function to handle environment variable aliases.
  • Applied aliases for various executor-related environment variables.
  • +18/-8   
    main.rs
    Update main configuration with new executor chain ID         

    src/main.rs

    • Updated chain_id to executor_chain_id in main configuration.
    +1/-1     
    Tests
    test_import_external_snapshot_common.rs
    Update test configuration with new executor fields             

    tests/test_import_external_snapshot_common.rs

  • Updated test configuration to use executor_chain_id and executor_evms.

  • +2/-2     

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

    Copy link

    PR Reviewer Guide 🔍

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

    Initialization Logging
    The initialization process, including the loading of dotenv and the application of environment variable aliases, should be logged to ensure visibility into the configuration process.

    Use of env::var and env::set_var
    Consider logging the environment variable changes made by the env_alias function to provide better traceability and debugging information.

    Copy link

    github-actions bot commented Jul 31, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Ensure that the env_alias function does not overwrite existing environment variables

    Add a check to ensure that the env_alias function does not overwrite existing
    environment variables.

    src/globals.rs [84-85]

     if let Ok(value) = env::var(alias) {
    -    env::set_var(canonical, value);
    +    if env::var(canonical).is_err() {
    +        env::set_var(canonical, value);
    +    }
     }
     
    Suggestion importance[1-10]: 10

    Why: Preventing env_alias from overwriting existing environment variables is crucial to avoid unintended side effects and maintain the integrity of the environment configuration.

    10
    Robustness
    Add error handling for the load_dotenv function to log a warning if the .env file cannot be loaded

    Add error handling for the load_dotenv function to log a warning if the .env file is
    not found or cannot be loaded.

    src/globals.rs [41]

    -load_dotenv();
    +if let Err(err) = load_dotenv() {
    +    eprintln!("Warning: Failed to load .env file: {}", err);
    +}
     
    Suggestion importance[1-10]: 9

    Why: Adding error handling for load_dotenv is important for robustness, as it informs the user of potential issues with environment configuration.

    9
    Maintainability
    Use a loop to apply environment variable aliases to reduce code repetition and improve maintainability

    Consider using a loop to apply environment variable aliases to reduce code
    repetition and improve maintainability.

    src/globals.rs [44-48]

    -env_alias("EXECUTOR_CHAIN_ID", "CHAIN_ID");
    -env_alias("EXECUTOR_EVMS", "EVMS");
    -env_alias("EXECUTOR_STRATEGY", "STRATEGY");
    -env_alias("TRACING_LOG_FORMAT", "LOG_FORMAT");
    -env_alias("TRACING_URL", "TRACING_COLLECTOR_URL");
    +let aliases = [
    +    ("EXECUTOR_CHAIN_ID", "CHAIN_ID"),
    +    ("EXECUTOR_EVMS", "EVMS"),
    +    ("EXECUTOR_STRATEGY", "STRATEGY"),
    +    ("TRACING_LOG_FORMAT", "LOG_FORMAT"),
    +    ("TRACING_URL", "TRACING_COLLECTOR_URL"),
    +];
    +for (canonical, alias) in &aliases {
    +    env_alias(canonical, alias);
    +}
     
    Suggestion importance[1-10]: 8

    Why: Using a loop to handle environment variable aliases reduces code repetition and improves maintainability, making the code cleaner and easier to update.

    8
    Best practice
    Add a default value for the num_evms field to ensure it has a valid value if not provided

    Consider adding a default value for the num_evms field to ensure it has a valid
    value if not provided by the user.

    src/eth/executor/executor_config.rs [21-22]

    -#[arg(long = "evms", env = "EXECUTOR_EVMS")]
    +#[arg(long = "evms", env = "EXECUTOR_EVMS", default_value_t = 1)]
     pub num_evms: usize,
     
    Suggestion importance[1-10]: 7

    Why: Adding a default value for num_evms improves robustness by ensuring it always has a valid value, but it is not critical since the user is expected to provide this value.

    7

    @dinhani-cw
    Copy link
    Contributor Author

    /describe

    Copy link

    PR Description updated to latest commit (01c6711)

    @dinhani-cw dinhani-cw enabled auto-merge (squash) July 31, 2024 09:14
    @dinhani-cw dinhani-cw merged commit 91a0b1e into main Jul 31, 2024
    33 checks passed
    @dinhani-cw dinhani-cw deleted the config-alias branch July 31, 2024 09:17
    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