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

fix accessing .Values in templates #2174

Merged
merged 1 commit into from
Mar 26, 2024
Merged

Conversation

maxmanuylov
Copy link
Contributor

@maxmanuylov maxmanuylov commented Mar 26, 2024

User description

Description

".Values" is incorrectly accessed in some templates (in those, for which the context is not global)

Motivation and Context

Right now browser nodes do not start because startup probe does not work because probe scripts are not mounted to the pod because ".Values.nodeConfigMap.extraScripts" is empty when template is called with non-global context.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

Type

bug_fix


Description

  • Fixed an issue where .Values was incorrectly accessed in autoscaling and video recorder configurations, causing browser nodes not to start.
  • Ensured autoscaling spec template correctly merges configurations and handles triggers by using global context.
  • Updated video recorder and uploader configurations to reference global .Values for consistency and correctness.
  • Corrected configurations related to image pull secrets, node selector, and volumes to properly use global .Values.

Changes walkthrough

Relevant files
Bug fix
_helpers.tpl
Fix Incorrect Access to .Values in Autoscaling and Video Recorder
Configurations

charts/selenium-grid/templates/_helpers.tpl

  • Fixed incorrect access to .Values in autoscaling and video recorder
    configurations by ensuring the context is global using $.
  • Adjusted autoscaling spec template to correctly merge configurations
    and handle triggers.
  • Updated video recorder and uploader configurations to use global
    context for .Values.
  • Corrected image pull secrets, node selector, and volumes configuration
    to properly reference global .Values.
  • +35/-35 

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

    @CLAassistant
    Copy link

    CLAassistant commented Mar 26, 2024

    CLA assistant check
    All committers have signed the CLA.

    Copy link

    qodo-merge-pro bot commented Mar 26, 2024

    PR Description updated to latest commit (fb42ab2)

    Copy link

    PR Review

    ⏱️ Estimated effort to review [1-5]

    2, because the changes are mostly about ensuring the correct scope is used for accessing values in Helm templates. The modifications are straightforward and follow a consistent pattern across the file.

    🧪 Relevant tests

    No

    🔍 Possible issues

    Context Scope: Ensure that the switch from local to global context ($) does not inadvertently affect other parts of the template where local context might be necessary.

    Hardcoded Values: The use of hardcoded values (e.g., triggerIndex: '{{ default $.Values.autoscaling.scaledOptions.minReplicaCount (.node.scaledOptions).minReplicaCount }}') could be less flexible for some configurations. Consider making these values more configurable if needed.

    🔒 Security concerns

    No


    ✨ Review tool usage guide:

    Overview:
    The review tool scans the PR code changes, and generates a PR review which includes several types of feedbacks, such as possible PR issues, security threats and relevant test in the PR. More feedbacks can be added by configuring the tool.

    The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR.

    • When commenting, to edit configurations related to the review tool (pr_reviewer section), use the following template:
    /review --pr_reviewer.some_config1=... --pr_reviewer.some_config2=...
    
    [pr_reviewer]
    some_config1=...
    some_config2=...
    

    See the review usage page for a comprehensive guide on using this tool.

    Copy link

    qodo-merge-pro bot commented Mar 26, 2024

    PR Code Suggestions

    CategorySuggestions                                                                                                                                                       
    Best practice
    Convert $spec back to a dictionary after toYaml to ensure correct merging.

    To ensure that the mergeOverwrite function correctly merges the dictionaries, it's
    recommended to explicitly convert the $spec variable back to a dictionary after the toYaml
    operation. This ensures that the mergeOverwrite function operates on dictionaries rather
    than strings, which is its intended use.

    charts/selenium-grid/templates/_helpers.tpl [137]

    -{{- $spec = mergeOverwrite ($spec | fromYaml) . | toYaml -}}
    +{{- $spec = mergeOverwrite ($spec | fromYaml) . | toYaml | fromYaml -}}
     
    
    Possible issue
    Validate existence of nested keys before access to avoid runtime errors.

    Consider validating the existence of $.Values.autoscaling.scaledOptions.triggers before
    attempting to access it to avoid potential runtime errors in case the triggers key is not
    defined in the values file.

    charts/selenium-grid/templates/_helpers.tpl [163]

    -{{- if not $.Values.autoscaling.scaledOptions.triggers }}
    +{{- if and $.Values.autoscaling.scaledOptions (not $.Values.autoscaling.scaledOptions.triggers) }}
     
    
    Maintainability
    Use a helper template for constructing image names for maintainability.

    For better maintainability and readability, consider using a helper template for
    constructing image names, which can be reused across different parts of the chart.

    charts/selenium-grid/templates/_helpers.tpl [213]

    -image: {{ printf "%s/%s:%s" $imageRegistry .node.imageName $imageTag }}
    +image: {{ include "seleniumGrid.constructImageName" (dict "registry" $imageRegistry "name" .node.imageName "tag" $imageTag) }}
     
    
    Enhancement
    Validate uploader configuration to prevent configuration errors.

    To ensure the video recorder's uploader component is correctly configured, validate that
    both $.Values.videoRecorder.uploader.enabled and $.Values.videoRecorder.uploader.name are
    set. This prevents the creation of an uploader container without a specified name, which
    could lead to configuration errors.

    charts/selenium-grid/templates/_helpers.tpl [397]

     {{- if and $.Values.videoRecorder.uploader.enabled (not (empty $.Values.videoRecorder.uploader.name)) }}
    +  # Ensure uploader configuration is valid here
    +{{- else }}
    +  # Handle invalid uploader configuration
    +{{- end }}
     
    
    Security
    Define a restrictive securityContext for containers to enhance security.

    To improve the security and restrict the permissions of the video recorder and uploader
    containers, consider defining a more restrictive securityContext. This can help in
    minimizing potential security risks.

    charts/selenium-grid/templates/_helpers.tpl [386]

    -securityContext: {{- toYaml . | nindent 10 }}
    +securityContext:
    +  runAsUser: 1000
    +  runAsNonRoot: true
    +  readOnlyRootFilesystem: true
    +  capabilities:
    +    drop:
    +      - ALL
     
    

    ✨ Improve tool usage guide:

    Overview:
    The improve tool scans the PR code changes, and automatically generates suggestions for improving the PR code. The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on a PR.

    • When commenting, to edit configurations related to the improve tool (pr_code_suggestions section), use the following template:
    /improve --pr_code_suggestions.some_config1=... --pr_code_suggestions.some_config2=...
    
    [pr_code_suggestions]
    some_config1=...
    some_config2=...
    

    See the improve usage page for a comprehensive guide on using this tool.

    @VietND96 VietND96 self-requested a review March 26, 2024 15:17
    @VietND96 VietND96 merged commit 6f03eb1 into SeleniumHQ:trunk Mar 26, 2024
    10 checks passed
    @VietND96
    Copy link
    Member

    @maxmanuylov, thank you for your interest in the chart and for providing a valuable fix!

    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.

    3 participants