Skip to content

feat(chart): add support for revisionhistorylimit #2339

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 8, 2024

Conversation

DrFaust92
Copy link
Contributor

@DrFaust92 DrFaust92 commented Aug 7, 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


Description

  • Added revisionHistoryLimit field to the deployment specs of Chrome node, distributor, Edge node, event bus, Firefox node, hub, router, session map, and session queue.
  • Added revisionHistoryLimit field to global values in values.yaml.
  • Changed single quotes to double quotes in various fields in values.yaml.

Changes walkthrough 📝

Relevant files
Enhancement
10 files
chrome-node-deployment.yaml
Add revision history limit to Chrome node deployment         

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

  • Added revisionHistoryLimit field to the deployment spec.
+1/-0     
distributor-deployment.yaml
Add revision history limit to distributor deployment         

charts/selenium-grid/templates/distributor-deployment.yaml

  • Added revisionHistoryLimit field to the deployment spec.
+1/-0     
edge-node-deployment.yaml
Add revision history limit to Edge node deployment             

charts/selenium-grid/templates/edge-node-deployment.yaml

  • Added revisionHistoryLimit field to the deployment spec.
+1/-0     
event-bus-deployment.yaml
Add revision history limit to event bus deployment             

charts/selenium-grid/templates/event-bus-deployment.yaml

  • Added revisionHistoryLimit field to the deployment spec.
+1/-0     
firefox-node-deployment.yaml
Add revision history limit to Firefox node deployment       

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

  • Added revisionHistoryLimit field to the deployment spec.
+1/-0     
hub-deployment.yaml
Add revision history limit to hub deployment                         

charts/selenium-grid/templates/hub-deployment.yaml

  • Added revisionHistoryLimit field to the deployment spec.
+1/-0     
router-deployment.yaml
Add revision history limit to router deployment                   

charts/selenium-grid/templates/router-deployment.yaml

  • Added revisionHistoryLimit field to the deployment spec.
+1/-0     
session-map-deployment.yaml
Add revision history limit to session map deployment         

charts/selenium-grid/templates/session-map-deployment.yaml

  • Added revisionHistoryLimit field to the deployment spec.
+1/-0     
session-queue-deployment.yaml
Add revision history limit to session queue deployment     

charts/selenium-grid/templates/session-queue-deployment.yaml

  • Added revisionHistoryLimit field to the deployment spec.
+1/-0     
values.yaml
Add revision history limit to global values and update quotes

charts/selenium-grid/values.yaml

  • Added revisionHistoryLimit field to global values.
  • Changed single quotes to double quotes in various fields.
  • +17/-17 

    💡 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 Aug 7, 2024

    CLA assistant check
    All committers have signed the CLA.

    Copy link

    qodo-merge-pro bot commented Aug 7, 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 7, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Best practice
    Add a conditional check for revisionHistoryLimit to ensure it is defined before usage

    Consider adding a conditional check to ensure that the revisionHistoryLimit value is
    defined in the values file before using it in the deployment template. This will
    prevent deployment errors in cases where the value is not specified.

    charts/selenium-grid/templates/chrome-node-deployment.yaml [21]

    +{{- if .Values.global.seleniumGrid.revisionHistoryLimit }}
     revisionHistoryLimit: {{ .Values.global.seleniumGrid.revisionHistoryLimit }}
    +{{- end }}
     
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: This suggestion is crucial for preventing deployment errors when revisionHistoryLimit is not defined. It ensures robustness and avoids potential failures.

    9
    Verify revisionHistoryLimit is set in values before applying it

    It's a good practice to check for the existence of configuration values before
    applying them. This prevents the deployment from failing due to missing values.

    charts/selenium-grid/templates/hub-deployment.yaml [19]

    +{{- if .Values.global.seleniumGrid.revisionHistoryLimit }}
     revisionHistoryLimit: {{ .Values.global.seleniumGrid.revisionHistoryLimit }}
    +{{- end }}
     
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: This suggestion follows best practices by ensuring that configuration values are checked for existence before being applied, preventing potential deployment failures.

    9
    Maintainability
    Conditionally set revisionHistoryLimit based on its presence in the values file

    To maintain consistency and avoid errors, ensure that the revisionHistoryLimit is
    only set when it is explicitly defined in the values file.

    charts/selenium-grid/templates/distributor-deployment.yaml [16]

    +{{- if .Values.global.seleniumGrid.revisionHistoryLimit }}
     revisionHistoryLimit: {{ .Values.global.seleniumGrid.revisionHistoryLimit }}
    +{{- end }}
     
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: This suggestion enhances maintainability and consistency by ensuring that revisionHistoryLimit is only set when explicitly defined, preventing potential deployment issues.

    9
    Possible issue
    Ensure revisionHistoryLimit is defined before setting it in the deployment

    To prevent potential deployment failures, add a check to ensure the
    revisionHistoryLimit is provided before setting it in the deployment configuration.

    charts/selenium-grid/templates/firefox-node-deployment.yaml [21]

    +{{- if .Values.global.seleniumGrid.revisionHistoryLimit }}
     revisionHistoryLimit: {{ .Values.global.seleniumGrid.revisionHistoryLimit }}
    +{{- end }}
     
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: This suggestion is important for preventing deployment failures by checking if revisionHistoryLimit is provided before using it in the configuration.

    9

    @VietND96 VietND96 merged commit 547f97e into SeleniumHQ:trunk Aug 8, 2024
    30 checks passed
    @VietND96
    Copy link
    Member

    VietND96 commented Aug 8, 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.

    3 participants