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

chart(add): Set pod/container name to node stereotypes #2323

Merged
merged 1 commit into from
Jul 24, 2024

Conversation

VietND96
Copy link
Member

@VietND96 VietND96 commented Jul 24, 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

chart(add): Set pod/container name to node stereotypes

Motivation and Context

se:containerName is added by default to Node stereotypes via env var SE_NODE_CONTAINER_NAME
This is useful in few scenarios as below:

  1. In Kubernetes deployment, we can track the node is running in which pod name by setting metadata.name to the env var. For example:
        env:
          - name: SE_NODE_CONTAINER_NAME
            valueFrom:
              fieldRef:
                fieldPath: metadata.name

Via Grid UI can see stereotypes

image

Session capabilities can see:

image

  1. In Docker deployment, we can set a unique name to that env var and setting custom capabilities for matching specific Nodes
environment:
  - SE_NODE_CONTAINER_NAME: this_specific_node_is_use
FirefoxOptions options = new FirefoxOptions();
options.setCapability("se:containerName", "this_specific_node_is_use");
options.setCapability("browserName", "chrome");
WebDriver driver = new RemoteWebDriver(new URL("http://localhost:4444"), options);
driver.get("https://selenium.dev");
driver.quit();

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

  • Dynamically fetch the latest Helm version if HELM_VERSION is set to "latest" in chart_setup_env.sh.
  • Added SE_NODE_CONTAINER_NAME environment variable to Kubernetes node configuration, setting it to the pod metadata name.
  • Updated Kubernetes and Helm versions in CircleCI configuration for various test workflows.
  • Updated Kubernetes and Helm versions in GitHub Actions for Helm chart tests.
  • Included se:containerName in the default node stereotype configuration in NodeBase/generate_config and Standalone/generate_config.

Changes walkthrough 📝

Relevant files
Enhancement
chart_setup_env.sh
Dynamically fetch the latest Helm version if specified     

tests/charts/make/chart_setup_env.sh

  • Updated Helm version handling to fetch the latest version dynamically.
  • Added logic to determine the latest Helm version if HELM_VERSION is
    set to "latest".
  • +4/-1     
    _helpers.tpl
    Add SE_NODE_CONTAINER_NAME environment variable to Kubernetes nodes

    charts/selenium-grid/templates/_helpers.tpl

  • Added environment variable SE_NODE_CONTAINER_NAME to Kubernetes node
    configuration.
  • Set SE_NODE_CONTAINER_NAME to Kubernetes pod metadata name.
  • +4/-0     
    generate_config
    Include container name in default node stereotype               

    NodeBase/generate_config

  • Added se:containerName to the default node stereotype configuration.
  • +1/-1     
    generate_config
    Include container name in default node stereotype               

    Standalone/generate_config

  • Added se:containerName to the default node stereotype configuration.
  • +1/-1     
    Configuration changes
    config.yml
    Update Kubernetes and Helm versions in CircleCI config     

    .circleci/config.yml

    • Updated Kubernetes and Helm versions for various test workflows.
    +5/-5     
    helm-chart-test.yml
    Update Kubernetes and Helm versions in GitHub Actions       

    .github/workflows/helm-chart-test.yml

    • Updated Kubernetes and Helm versions for Helm chart tests.
    +5/-5     

    💡 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: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Script Robustness
    The script uses curl to fetch the latest Helm version without error handling. If the curl or grep command fails, it might set an empty or incorrect HELM_VERSION, which could lead to further issues in the script execution. Consider adding error handling after the curl command to ensure that a valid version is fetched and processed.

    Environment Variable Injection
    The addition of SE_NODE_CONTAINER_NAME environment variable is hardcoded to use metadata.name. This might not be flexible for different deployment scenarios where a different container naming convention is needed. Consider making this configurable through values in the Helm chart.

    Code Duplication
    The SE_NODE_STEREOTYPE JSON string is manually constructed in multiple places (NodeBase/generate_config and Standalone/generate_config). This could lead to inconsistencies and maintenance issues. Consider centralizing the construction of this JSON in a shared utility function or script.

    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add error handling for the curl command fetching the latest Helm version

    To ensure the script is robust against potential failures when fetching the latest
    Helm version, it's advisable to check the success of the curl command before
    proceeding. This can be achieved by checking the exit status of the curl command. If
    the command fails, the script should handle this gracefully, possibly by reverting
    to a default version of Helm or by exiting with an error message.

    tests/charts/make/chart_setup_env.sh [124]

     HELM_VERSION=$(curl -s https://api.github.com/repos/helm/helm/releases/latest | grep tag_name | cut -d '"' -f 4)
    +if [ $? -ne 0 ]; then
    +    echo "Error fetching latest Helm version. Reverting to default version."
    +    HELM_VERSION="v3.15.2"
    +fi
     
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: This suggestion adds robust error handling for the curl command, ensuring the script can handle failures gracefully. This is crucial for maintaining script reliability.

    9
    Best practice
    Use jq to dynamically generate the JSON string for SE_NODE_STEREOTYPE

    The JSON string for SE_NODE_STEREOTYPE should properly handle JSON encoding to avoid
    issues with special characters in variable values. Using a tool like jq can help
    create a valid JSON string dynamically.

    NodeBase/generate_config [67]

    -SE_NODE_STEREOTYPE="{\"browserName\": \"${SE_NODE_BROWSER_NAME}\", \"browserVersion\": \"${SE_NODE_BROWSER_VERSION}\", \"platformName\": \"Linux\", ${SE__BROWSER_BINARY_LOCATION}, \"se:containerName\": \"${SE_NODE_CONTAINER_NAME}\"}"
    +SE_NODE_STEREOTYPE=$(jq -n \
    +                      --arg bn "$SE_NODE_BROWSER_NAME" \
    +                      --arg bv "$SE_NODE_BROWSER_VERSION" \
    +                      --arg pn "Linux" \
    +                      --arg bl "$SE__BROWSER_BINARY_LOCATION" \
    +                      --arg cn "$SE_NODE_CONTAINER_NAME" \
    +                      '{browserName: $bn, browserVersion: $bv, platformName: $pn, browserBinaryLocation: $bl, "se:containerName": $cn}')
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Using jq ensures proper JSON encoding and handling of special characters, which is a best practice for generating JSON strings dynamically.

    8
    Implement jq for JSON string creation in SE_NODE_STEREOTYPE to ensure proper encoding

    Similar to the suggestion for NodeBase/generate_config, using jq for generating JSON
    ensures proper encoding and handling of special characters in the SE_NODE_STEREOTYPE
    variable.

    Standalone/generate_config [40]

    -SE_NODE_STEREOTYPE="{\"browserName\": \"${SE_NODE_BROWSER_NAME}\", \"browserVersion\": \"${SE_NODE_BROWSER_VERSION}\", \"platformName\": \"Linux\", ${SE__BROWSER_BINARY_LOCATION}, \"se:containerName\": \"${SE_NODE_CONTAINER_NAME}\"}"
    +SE_NODE_STEREOTYPE=$(jq -n \
    +                      --arg bn "$SE_NODE_BROWSER_NAME" \
    +                      --arg bv "$SE_NODE_BROWSER_VERSION" \
    +                      --arg pn "Linux" \
    +                      --arg bl "$SE__BROWSER_BINARY_LOCATION" \
    +                      --arg cn "$SE_NODE_CONTAINER_NAME" \
    +                      '{browserName: $bn, browserVersion: $bv, platformName: $pn, browserBinaryLocation: $bl, "se:containerName": $cn}')
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Similar to the previous suggestion, using jq ensures proper JSON encoding and handling of special characters, improving the reliability of the script.

    8
    Enhancement
    Add a conditional check for metadata.name before setting SE_NODE_CONTAINER_NAME

    It is recommended to use a conditional check to ensure that the metadata.name field
    is available before using it to set the SE_NODE_CONTAINER_NAME. This can prevent
    potential runtime errors in environments where metadata might not be fully available
    or in expected format.

    charts/selenium-grid/templates/_helpers.tpl [253-256]

    +{{- if .node.metadata.name }}
     - name: SE_NODE_CONTAINER_NAME
       valueFrom:
         fieldRef:
           fieldPath: metadata.name
    +{{- end }}
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Adding a conditional check for metadata.name can prevent potential runtime errors, enhancing the robustness of the template. However, the likelihood of metadata.name being unavailable is low.

    7

    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