You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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(breaking change): update config key to enable ingress
Motivation and Context
Config keys are changed as below
ingress.enabled changes to ingress.enableWithExistingController: Enable or disable ingress resource (without installing Ingress NGINX Controller subchart)
Now ingress.enabled to used to enable ingress resource. Implies installing Ingress NGINX Controller
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)
Ingress Condition The condition for enabling ingress has been updated to check both ingress.enabled and ingress.enableWithExistingController. Ensure that this logic correctly handles all intended use cases and does not unintentionally enable or disable ingress.
Default Values The default values for ingress.enabled and ingress.enableWithExistingController have been changed. This might affect existing deployments that rely on the previous default settings. Ensure that these changes are clearly documented and communicated to users to avoid breaking changes.
Conditional Steps Several steps in the GitHub Actions workflow now include conditions based on the skip-build-push-image input. Verify that these conditions are correctly set to avoid skipping necessary build steps unintentionally.
Add a condition to prevent unintended deployments in GitHub Actions workflow
Add a condition to check if the skip-build-push-image input is 'true' before running the 'Deploy new images' job to prevent unintended deployments when image building and pushing is skipped.
Why: This suggestion prevents unintended deployments in the GitHub Actions workflow, addressing a possible issue that could lead to incorrect deployments.
9
Security
Ensure proper quoting of variables in shell scripts to prevent security issues
Ensure that the NAMESPACE variable is properly quoted to prevent globbing and word splitting issues, which can lead to unexpected behavior or security vulnerabilities.
-echo "### Published Docker images on [Docker Hub](https://hub.docker.com/u/${NAMESPACE})" >> release_notes.md+echo "### Published Docker images on [Docker Hub](https://hub.docker.com/u/\"${NAMESPACE}\")" >> release_notes.md
Apply this suggestion
Suggestion importance[1-10]: 8
Why: Properly quoting variables in shell scripts is important for preventing security issues and unexpected behavior. This suggestion addresses a potential security concern.
8
Best practice
Ensure consistent credential handling across architectures in Docker commands
Add the missing --credential yes option to the docker run command for the non-amd64 architecture cases to ensure consistency in handling credentials across different architectures.
-docker run --rm --privileged aptman/qus -- -r ;-docker run --rm --privileged aptman/qus -s -- -p+docker run --rm --privileged aptman/qus -- -r --credential yes;+docker run --rm --privileged aptman/qus -s -- -p --credential yes
Apply this suggestion
Suggestion importance[1-10]: 7
Why: Adding the --credential yes option ensures consistent credential handling across different architectures, which is a good practice for maintaining uniform behavior.
7
Maintainability
Use a separate variable for URLs to ensure proper handling of special characters and dynamic content
Replace the hardcoded selenium-grid-${chart_version} with a variable that includes the full URL, ensuring it is dynamically generated and properly escaped to handle special characters in the version string.
-echo "### Published Helm chart version [selenium-grid-${chart_version}](https://github.com/${AUTHORS:-"SeleniumHQ"}/docker-selenium/releases/tag/selenium-grid-${chart_version})" >> release_notes.md+full_chart_url="https://github.com/${AUTHORS:-"SeleniumHQ"}/docker-selenium/releases/tag/selenium-grid-${chart_version}"+echo "### Published Helm chart version [selenium-grid-${chart_version}](${full_chart_url})" >> release_notes.md
Apply this suggestion
Suggestion importance[1-10]: 6
Why: Using a separate variable for URLs improves maintainability and ensures proper handling of special characters, though the improvement is minor.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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(breaking change): update config key to enable ingress
Motivation and Context
Config keys are changed as below
ingress.enabled
changes toingress.enableWithExistingController
: Enable or disable ingress resource (without installing Ingress NGINX Controller subchart)Now
ingress.enabled
to used to enable ingress resource. Implies installing Ingress NGINX ControllerTypes of changes
Checklist
PR Type
Enhancement, Tests, Documentation
Description
--credential yes
flag todocker run
command in chart setup script.RENDER_HELM_TEMPLATE_ONLY
variable for conditional execution in chart test script.Changes walkthrough 📝
14 files
generate_release_notes.sh
Enhance release notes with Helm chart version and Docker Hub link
generate_release_notes.sh
chart_setup_env.sh
Add credential flag to multiarch/qemu-user-static docker run command
tests/charts/make/chart_setup_env.sh
--credential yes
flag todocker run
command formultiarch/qemu-user-static.
chart_test.sh
Add conditional execution for Helm template rendering
tests/charts/make/chart_test.sh
RENDER_HELM_TEMPLATE_ONLY
variable for conditionalexecution.
RENDER_HELM_TEMPLATE_ONLY
._helpers.tpl
Add helper functions for ingress and monitoring enablement
charts/selenium-grid/templates/_helpers.tpl
checks.
deploy.yml
Add option to skip image build and push in deploy workflow
.github/workflows/deploy.yml
skip-build-push-image
input to skip image build and push steps.docker-test.yml
Add disk space cleanup step in docker-test workflow
.github/workflows/docker-test.yml
helm-chart-test.yml
Add disk space cleanup step in helm-chart-test workflow
.github/workflows/helm-chart-test.yml
rerun-failed.yml
Add write-all permissions to rerun-failed workflow
.github/workflows/rerun-failed.yml
permissions: write-all
to the workflow.Makefile
Add chart_render_template target and update chart test targets
Makefile
chart_render_template
target for rendering Helm templates.TEMPLATE_OUTPUT_FILENAME
.Chart.yaml
Update dependency conditions with new enablement flags
charts/selenium-grid/Chart.yaml
NOTES.txt
Update ingress enablement check in NOTES.txt
charts/selenium-grid/templates/NOTES.txt
ingress.yaml
Update ingress enablement check in ingress.yaml
charts/selenium-grid/templates/ingress.yaml
values.yaml
Add new flags for ingress and monitoring configurations
charts/selenium-grid/values.yaml
enableWithExistingController
flag for ingress.enabledWithExistingAgent
flag for monitoring.base-auth-ingress-values.yaml
Add enabled flag for ingress in base-auth-ingress-values.yaml
tests/charts/ci/base-auth-ingress-values.yaml
enabled
flag for ingress.1 files
README.md
Update README with new ingress and tracing configurations
charts/selenium-grid/README.md
configurations.