Skip to content

feat: add support for structure logs #2342

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

Merged
merged 2 commits into from
Aug 9, 2024

Conversation

DrFaust92
Copy link
Contributor

@DrFaust92 DrFaust92 commented Aug 8, 2024

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

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

Enhancement, Configuration changes


Description

  • Added support for --structured-logs option across multiple Selenium Grid components.
  • Introduced new environment variable SE_STRUCTURED_LOGS to enable structured logging.
  • Updated ConfigMap and values.yaml to include structuredLogs configuration.

Changes walkthrough 📝

Relevant files
Enhancement
10 files
start-selenium-grid-distributor.sh
Add support for structured logs in Distributor script       

Distributor/start-selenium-grid-distributor.sh

  • Added support for --structured-logs option.
  • Appended new environment variable SE_STRUCTURED_LOGS.
  • +5/-0     
    start-selenium-grid-eventbus.sh
    Add support for structured logs in EventBus script             

    EventBus/start-selenium-grid-eventbus.sh

  • Added support for --structured-logs option.
  • Appended new environment variable SE_STRUCTURED_LOGS.
  • +5/-0     
    start-selenium-grid-hub.sh
    Add support for structured logs in Hub script                       

    Hub/start-selenium-grid-hub.sh

  • Added support for --structured-logs option.
  • Appended new environment variable SE_STRUCTURED_LOGS.
  • +5/-0     
    start-selenium-node.sh
    Add support for structured logs in NodeBase script             

    NodeBase/start-selenium-node.sh

  • Added support for --structured-logs option.
  • Appended new environment variable SE_STRUCTURED_LOGS.
  • +5/-0     
    start-selenium-grid-docker.sh
    Add support for structured logs in NodeDocker script         

    NodeDocker/start-selenium-grid-docker.sh

  • Added support for --structured-logs option.
  • Appended new environment variable SE_STRUCTURED_LOGS.
  • +5/-0     
    start-selenium-grid-router.sh
    Add support for structured logs in Router script                 

    Router/start-selenium-grid-router.sh

  • Added support for --structured-logs option.
  • Appended new environment variable SE_STRUCTURED_LOGS.
  • +5/-0     
    start-selenium-grid-session-queue.sh
    Add support for structured logs in SessionQueue script     

    SessionQueue/start-selenium-grid-session-queue.sh

  • Added support for --structured-logs option.
  • Appended new environment variable SE_STRUCTURED_LOGS.
  • +5/-0     
    start-selenium-grid-sessions.sh
    Add support for structured logs in Sessions script             

    Sessions/start-selenium-grid-sessions.sh

  • Added support for --structured-logs option.
  • Appended new environment variable SE_STRUCTURED_LOGS.
  • +5/-0     
    start-selenium-standalone.sh
    Add support for structured logs in Standalone script         

    Standalone/start-selenium-standalone.sh

  • Added support for --structured-logs option.
  • Appended new environment variable SE_STRUCTURED_LOGS.
  • +5/-0     
    start-selenium-grid-docker.sh
    Add support for structured logs in StandaloneDocker script

    StandaloneDocker/start-selenium-grid-docker.sh

  • Added support for --structured-logs option.
  • Appended new environment variable SE_STRUCTURED_LOGS.
  • +5/-0     
    Configuration changes
    2 files
    logging-configmap.yaml
    Add structured logs configuration to ConfigMap                     

    charts/selenium-grid/templates/logging-configmap.yaml

    • Added SE_STRUCTURED_LOGS configuration to ConfigMap.
    +1/-0     
    values.yaml
    Add structured logs field to global values                             

    charts/selenium-grid/values.yaml

    • Added structuredLogs field to global values.
    +2/-0     

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

    Copy link

    qodo-merge-pro bot commented Aug 8, 2024

    PR Reviewer Guide 🔍

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

    Copy link

    qodo-merge-pro bot commented Aug 8, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Security
    Sanitize or validate SE_STRUCTURED_LOGS to prevent command injection

    To avoid potential command injection vulnerabilities, ensure that SE_STRUCTURED_LOGS
    is properly sanitized or validated before use in the script.

    EventBus/start-selenium-grid-eventbus.sh [27-29]

    -if [ ! -z "$SE_STRUCTURED_LOGS" ]; then
    +if [ ! -z "$SE_STRUCTURED_LOGS" ] && printf '%s' "$SE_STRUCTURED_LOGS" | grep -qE '^[a-zA-Z0-9]+$'; then
       echo "Appending Selenium options: --structured-logs ${SE_STRUCTURED_LOGS}"
       SE_OPTS="$SE_OPTS --structured-logs ${SE_STRUCTURED_LOGS}"
     fi
     
    • Apply this suggestion
    Suggestion importance[1-10]: 10

    Why: This suggestion addresses a critical security concern by preventing command injection vulnerabilities, which is essential for maintaining the security and integrity of the system.

    10
    Best practice
    ✅ Ensure consistency in data types for configuration values
    Suggestion Impact:The commit changed the default value for SE_STRUCTURED_LOGS from a quoted string "false" to an unquoted boolean false, as suggested.

    code diff:

    -  SE_STRUCTURED_LOGS: "{{ default "false" .Values.global.seleniumGrid.structuredLogs }}"
    +  SE_STRUCTURED_LOGS: "{{ default false .Values.global.seleniumGrid.structuredLogs }}"

    Ensure that the default value for SE_STRUCTURED_LOGS is consistent with the expected
    data type in the scripts. If the scripts expect a boolean, the default should be a
    boolean (true or false) without quotes.

    charts/selenium-grid/templates/logging-configmap.yaml [16]

    -SE_STRUCTURED_LOGS: "{{ default "false" .Values.global.seleniumGrid.structuredLogs }}"
    +SE_STRUCTURED_LOGS: {{ default false .Values.global.seleniumGrid.structuredLogs }}
     
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: Ensuring consistency in data types between configuration files and scripts is crucial for preventing runtime errors and misconfigurations. This suggestion addresses a potential issue with data type mismatch.

    9
    Possible issue
    Validate the value of SE_STRUCTURED_LOGS before use

    Add a check to ensure that the value of SE_STRUCTURED_LOGS is either 'true' or
    'false' before appending it to SE_OPTS, to prevent potential misconfigurations.

    Distributor/start-selenium-grid-distributor.sh [62-64]

    -if [ ! -z "$SE_STRUCTURED_LOGS" ]; then
    +if [ ! -z "$SE_STRUCTURED_LOGS" ] && ([ "$SE_STRUCTURED_LOGS" = "true" ] || [ "$SE_STRUCTURED_LOGS" = "false" ]); then
       echo "Appending Selenium options: --structured-logs ${SE_STRUCTURED_LOGS}"
       SE_OPTS="$SE_OPTS --structured-logs ${SE_STRUCTURED_LOGS}"
     fi
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Validating the value of SE_STRUCTURED_LOGS before use can prevent misconfigurations and potential errors, making the script more robust and reliable.

    8
    Maintainability
    Clarify the expected data type in configuration comments

    Add a comment to clarify the expected data type for structuredLogs to ensure it
    aligns with usage in shell scripts and other configurations.

    charts/selenium-grid/values.yaml [33]

    -# Whether to enable structured logging
    +# Whether to enable structured logging (boolean)
     structuredLogs: false
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Adding comments to clarify expected data types improves code maintainability and helps future developers understand the configuration requirements, although it is a minor improvement.

    7

    @VietND96 VietND96 merged commit 68ebfe1 into SeleniumHQ:trunk Aug 9, 2024
    30 checks passed
    @VietND96
    Copy link
    Member

    VietND96 commented Aug 9, 2024

    Thank you, @DrFaust92 !

    @VietND96 VietND96 added this to the 4.23.1 milestone Aug 9, 2024
    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.

    2 participants