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

[docs] Generate list environment variables README automatically #2564

Merged
merged 4 commits into from
Jan 8, 2025

Conversation

VietND96
Copy link
Member

@VietND96 VietND96 commented Jan 6, 2025

User description

Thanks for contributing to the Docker-Selenium project!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines, applied for this repository.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

Checkout full list of environment variables here.

How to update or contribute to list of environment variables? Follow below steps:

  1. Refresh the list to pick up new environment variables or default value

    make update_list_env_vars

    The script can be updated in scripts/generate_list_env_vars/extract_env.py.

  2. Update the description for each environment variable in the file scripts/generate_list_env_vars/description.yaml.

  3. Run the command in step (1) one more time to update the list of environment variables with new descriptions.

Motivation and Context

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

PR Type

Documentation, Enhancement


Description

  • Added a script to extract and document environment variables.

  • Generated ENV_VARIABLES.md with a comprehensive list of environment variables.

  • Updated README.md to include a link to the environment variables documentation.

  • Enhanced the build process with a new Makefile target for updating environment variables.


Changes walkthrough 📝

Relevant files
Enhancement
bootstrap.sh
Use `requirements.txt` for Python dependencies                     

tests/charts/bootstrap.sh

  • Replaced individual package installation with requirements.txt.
  • Simplified dependency management for Python packages.
  • +1/-2     
    extract_env.py
    Script to extract and document environment variables         

    scripts/generate_list_env_vars/extract_env.py

  • Added script to extract environment variables from shell scripts and
    Dockerfiles.
  • Combined variables into YAML files for documentation.
  • Generated markdown table for environment variables.
  • +113/-0 
    Makefile
    Add Makefile target for environment variable updates         

    Makefile

  • Added a new target update_list_env_vars for generating environment
    variable documentation.
  • Integrated Python script execution into the build process.
  • +4/-0     
    Documentation
    ENV_VARIABLES.md
    Generated markdown for environment variables                         

    ENV_VARIABLES.md

  • Added a markdown file listing all environment variables.
  • Included variable names, default values, descriptions, and CLI
    options.
  • +121/-0 
    README.md
    Link to environment variables documentation in README       

    README.md

  • Added a section linking to the environment variables documentation.
  • Improved navigation for users to find environment variable details.
  • +5/-0     
    description.yaml
    YAML file for environment variable descriptions                   

    scripts/generate_list_env_vars/description.yaml

  • Added YAML file with descriptions for environment variables.
  • Included CLI options and descriptions for each variable.
  • +361/-0 
    value.yaml
    YAML file for environment variable default values               

    scripts/generate_list_env_vars/value.yaml

  • Added YAML file with default values for environment variables.
  • Structured data for easy reference and updates.
  • +238/-0 
    Dependencies
    requirements.txt
    Update test requirements with `pyyaml`                                     

    tests/requirements.txt

  • Added pyyaml as a dependency for the new script.
  • Updated requirements for testing and documentation generation.
  • +1/-0     

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

    Copy link

    qodo-merge-pro bot commented Jan 6, 2025

    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

    Error Handling

    The script silently continues on file read errors, which could lead to missing environment variables in the documentation. Consider adding error logging or failing the script on critical errors.

    except Exception as e:
        print(f"Error reading file {file_path}: {e}")
    Code Duplication

    Similar error handling patterns are duplicated in both extract functions. Consider refactoring common file reading logic into a separate function.

    except Exception as e:
        print(f"Error reading file {file_path}: {e}")

    Copy link

    qodo-merge-pro bot commented Jan 6, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Security
    Add protection for sensitive environment variables to prevent exposure of secrets

    The script processes sensitive environment variables without any filtering. Add
    logic to exclude or mask sensitive values (like passwords and keys).

    scripts/generate_list_env_vars/extract_env.py [23-28]

     def extract_variables_from_dockerfiles(directory_path):
    +    sensitive_vars = {'SE_ROUTER_PASSWORD', 'SE_VNC_PASSWORD', 'SE_JAVA_SSL_TRUST_STORE_PASSWORD'}
         variables = OrderedDict()
         for root, _, files in os.walk(directory_path):
             for file in files:
                 if file.startswith("Dockerfile"):
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: This is a critical security enhancement that prevents potential exposure of sensitive credentials and passwords in the generated documentation.

    8
    Possible issue
    Add proper error handling for file operations to prevent silent failures

    Add error handling for file operations when reading/writing YAML files. The current
    implementation could fail silently if files are missing or have incorrect
    permissions.

    scripts/generate_list_env_vars/extract_env.py [46-49]

     def read_description_yaml(file_path):
    -    with open(file_path, 'r') as file:
    -        data = yaml.safe_load(file)
    -        return data if data is not None else []
    +    try:
    +        with open(file_path, 'r') as file:
    +            data = yaml.safe_load(file)
    +            return data if data is not None else []
    +    except (IOError, yaml.YAMLError) as e:
    +        print(f"Error reading YAML file {file_path}: {e}")
    +        return []
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion adds important error handling for file operations that could fail silently. This improves robustness and helps with debugging by providing clear error messages.

    7
    General
    Improve regex pattern to correctly match all valid environment variable names

    The regex pattern for matching environment variables could miss some valid variable
    names. Update it to handle all valid environment variable name formats.

    scripts/generate_list_env_vars/extract_env.py [15]

    -pattern = r'\b(SE_[A-Za-z0-9_]+)\b'
    +pattern = r'\b(SE_[A-Za-z][A-Za-z0-9_]*)\b'
    • Apply this suggestion
    Suggestion importance[1-10]: 4

    Why: The improved regex pattern is slightly more precise by ensuring variables start with a letter, but the original pattern was already adequate for the SE_ prefixed variables in this context.

    4

    Copy link

    qodo-merge-pro bot commented Jan 8, 2025

    CI Failure Feedback 🧐

    (Checks updated until commit 7b687c2)

    Action: Rerun workflow when failure

    Failed stage: Authenticate GitHub CLI for PR [❌]

    Failure summary:

    The action failed because the GitHub authentication token lacks the required 'read:org' permission
    scope. The error occurred during the gh auth login command when trying to authenticate using
    GH_CLI_TOKEN_PR.

    Relevant error logs:
    1:  ##[group]Operating System
    2:  Ubuntu
    ...
    
    28:  SecurityEvents: write
    29:  Statuses: write
    30:  ##[endgroup]
    31:  Secret source: Actions
    32:  Prepare workflow directory
    33:  Prepare all required actions
    34:  Getting action download info
    35:  Download action repository 'actions/checkout@main' (SHA:cbb722410c2e876e24abbe8de2cc27693e501dcb)
    36:  Complete job name: Rerun workflow when failure
    ...
    
    48:  show-progress: true
    49:  lfs: false
    50:  submodules: false
    51:  set-safe-directory: true
    52:  env:
    53:  GH_CLI_TOKEN: ***
    54:  GH_CLI_TOKEN_PR: ***
    55:  RUN_ID: 12663004594
    56:  RERUN_FAILED_ONLY: true
    ...
    
    119:  ##[group]Run sudo apt update
    120:  �[36;1msudo apt update�[0m
    121:  �[36;1msudo apt install gh�[0m
    122:  shell: /usr/bin/bash -e {0}
    123:  env:
    124:  GH_CLI_TOKEN: ***
    125:  GH_CLI_TOKEN_PR: ***
    126:  RUN_ID: 12663004594
    127:  RERUN_FAILED_ONLY: true
    ...
    
    175:  0 upgraded, 0 newly installed, 0 to remove and 53 not upgraded.
    176:  ##[group]Run echo "$GH_CLI_TOKEN_PR" | gh auth login --with-token
    177:  �[36;1mecho "$GH_CLI_TOKEN_PR" | gh auth login --with-token�[0m
    178:  shell: /usr/bin/bash -e {0}
    179:  env:
    180:  GH_CLI_TOKEN: ***
    181:  GH_CLI_TOKEN_PR: ***
    182:  RUN_ID: 12663004594
    183:  RERUN_FAILED_ONLY: true
    184:  RUN_ATTEMPT: 1
    185:  ##[endgroup]
    186:  error validating token: missing required scope 'read:org'
    187:  ##[error]Process completed with exit code 1.
    

    ✨ CI feedback usage guide:

    The CI feedback tool (/checks) automatically triggers when a PR has a failed check.
    The tool analyzes the failed checks and provides several feedbacks:

    • Failed stage
    • Failed test name
    • Failure summary
    • Relevant error logs

    In addition to being automatically triggered, the tool can also be invoked manually by commenting on a PR:

    /checks "https://github.com/{repo_name}/actions/runs/{run_number}/job/{job_number}"
    

    where {repo_name} is the name of the repository, {run_number} is the run number of the failed check, and {job_number} is the job number of the failed check.

    Configuration options

    • enable_auto_checks_feedback - if set to true, the tool will automatically provide feedback when a check is failed. Default is true.
    • excluded_checks_list - a list of checks to exclude from the feedback, for example: ["check1", "check2"]. Default is an empty list.
    • enable_help_text - if set to true, the tool will provide a help message with the feedback. Default is true.
    • persistent_comment - if set to true, the tool will overwrite a previous checks comment with the new feedback. Default is true.
    • final_update_message - if persistent_comment is true and updating a previous checks message, the tool will also create a new message: "Persistent checks updated to latest commit". Default is true.

    See more information about the checks tool in the docs.

    @VietND96 VietND96 merged commit 98dbc41 into trunk Jan 8, 2025
    34 of 36 checks passed
    @VietND96 VietND96 deleted the list-env-vars branch January 8, 2025 12:35
    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.

    1 participant