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

K8s: regression issue on template for nodes service.enabled #2484

Merged
merged 2 commits into from
Dec 2, 2024

Conversation

VietND96
Copy link
Member

@VietND96 VietND96 commented Dec 2, 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

Bug fix


Description

  • Fixed a regression issue in the Kubernetes templates for Chrome and Firefox node services by correcting the context used for namespace and label references.
  • Added service.enabled field to the test configurations for Chrome, Firefox, and Edge nodes to ensure services are enabled as expected.

Changes walkthrough 📝

Relevant files
Bug fix
chrome-node-service.yaml
Fix namespace and label context in Chrome node service template

charts/selenium-grid/templates/chrome-node-service.yaml

  • Fixed namespace reference to use root context.
  • Corrected label inclusion to use root context.
  • Updated selector to use root context for release name.
  • +3/-3     
    firefox-node-service.yaml
    Fix label context in Firefox node service template             

    charts/selenium-grid/templates/firefox-node-service.yaml

    • Corrected label inclusion to use root context.
    +1/-1     
    Tests
    dummy.yaml
    Add service enabled field in dummy test configuration       

    tests/charts/templates/render/dummy.yaml

    • Added service.enabled field for Chrome, Firefox, and Edge nodes.
    +6/-0     
    dummy_solution.yaml
    Add service enabled field in dummy solution configuration

    tests/charts/templates/render/dummy_solution.yaml

    • Added service.enabled field for Chrome, Firefox, and Edge nodes.
    +6/-0     

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

    @VietND96 VietND96 linked an issue Dec 2, 2024 that may be closed by this pull request
    Copy link

    qodo-merge-pro bot commented Dec 2, 2024

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Context Reference
    Verify that using $.Release.Namespace and $.Release.Name instead of .Release.Namespace and .Release.Name maintains proper scoping and variable access in the Kubernetes template

    Configuration Change
    Validate that adding service.enabled=true for Chrome, Firefox and Edge nodes doesn't conflict with existing configurations and works as expected

    Copy link

    qodo-merge-pro bot commented Dec 2, 2024

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Respect the service.enabled configuration flag to conditionally create the Kubernetes service

    Add a conditional check to ensure the service is only created when service.enabled
    is true, as this field has been added in the configuration but not handled in the
    template.

    charts/selenium-grid/templates/chrome-node-service.yaml [4-7]

    +{{- if $nodeConfig.service.enabled }}
     apiVersion: v1
     kind: Service
     metadata:
       name: {{ template "seleniumGrid.chromeNode.fullname" (list $nodeConfig $) }}
    +{{- end }}
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: This is a critical suggestion that prevents creating unnecessary Kubernetes services when they're not needed. The PR adds the service.enabled configuration but doesn't implement the conditional creation, which could lead to resource waste and confusion.

    9

    💡 Need additional feedback ? start a PR chat

    Copy link

    qodo-merge-pro bot commented Dec 2, 2024

    CI Failure Feedback 🧐

    (Checks updated until commit 225a17d)

    Action: Rerun workflow when failure

    Failed stage: Authenticate GitHub CLI for PR [❌]

    Failure summary:

    The action failed due to an authentication error with the GitHub CLI:

  • The token used for authentication is missing the required scope read:org.
  • This resulted in the process terminating with an exit code 1.

  • 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: 12113417932
    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: 12113417932
    127:  RERUN_FAILED_ONLY: true
    ...
    
    160:  0 upgraded, 0 newly installed, 0 to remove and 41 not upgraded.
    161:  ##[group]Run echo "$GH_CLI_TOKEN_PR" | gh auth login --with-token
    162:  �[36;1mecho "$GH_CLI_TOKEN_PR" | gh auth login --with-token�[0m
    163:  shell: /usr/bin/bash -e {0}
    164:  env:
    165:  GH_CLI_TOKEN: ***
    166:  GH_CLI_TOKEN_PR: ***
    167:  RUN_ID: 12113417932
    168:  RERUN_FAILED_ONLY: true
    169:  RUN_ATTEMPT: 1
    170:  ##[endgroup]
    171:  error validating token: missing required scope 'read:org'
    172:  ##[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 cf0c839 into trunk Dec 2, 2024
    32 of 53 checks passed
    @VietND96 VietND96 deleted the fix-template branch December 2, 2024 07:58
    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.

    [🐛 Bug]: chromeNode.service.enabled regression
    1 participant