Closed VietND96 closed 3 weeks ago
Here are some key observations to aid the review process:
**π« Ticket compliance analysis πΆ** **[2111](https://github.com/SeleniumHQ/docker-selenium/issues/2111) - Partially compliant** Fully compliant requirements: - Support custom node types besides chrome/firefox/edge in helm chart - Allow specifying custom image for nodes Not compliant requirements: - Allow running mobile tests or tests against another browser supported by Selenium - Allow setting browserName for custom nodes (e.g., 'android') |
β±οΈ Estimated effort to review: 4 π΅π΅π΅π΅βͺ |
π§ͺ PR contains tests |
π No security concerns identified |
β‘ Recommended focus areas for review Configuration Complexity The relay node configuration in values.yaml is extensive and may require careful review to ensure all options are correctly set and documented. Deployment Strategy Verify that the deployment strategy for relay nodes is appropriate and consistent with other node types in the chart. Test Configuration Ensure that the docker-compose test configuration for relay nodes is comprehensive and covers all necessary scenarios. |
Explore these optional code suggestions:
Category | Suggestion | Score |
Security |
Add a default security context for the relay node deployment___ **Consider adding a default security context for the relay node to improve thesecurity posture of the deployment.** [charts/selenium-grid/values.yaml [1506-1507]](https://github.com/SeleniumHQ/docker-selenium/pull/2453/files#diff-78eaafaacc320a0b69216c3173a3961d2305653ce2c9f054fb4d42d90f71813eR1506-R1507) ```diff # -- SecurityContext for relay-node container -securityContext: {} +securityContext: + runAsNonRoot: true + runAsUser: 1000 + readOnlyRootFilesystem: true ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 9Why: Implementing a security context with settings like `runAsNonRoot` and `readOnlyRootFilesystem` significantly enhances the security posture of the deployment, making it a highly relevant and impactful suggestion. | 9 |
Best practice |
Add resource limits and requests for the relay node deployment___ **Consider adding resource limits and requests for the relay node deployment to ensureproper resource allocation and prevent resource exhaustion.** [charts/selenium-grid/templates/relay-node-deployment.yaml [35]](https://github.com/SeleniumHQ/docker-selenium/pull/2453/files#diff-efeb7430aadcf5ced25ab3747871f692cf846e870c59e1c6d89a5b5e304ea60cR35-R35) ```diff {{- include "seleniumGrid.podTemplate" $podScope | nindent 2 }} + resources: + limits: + cpu: {{ .Values.relayNode.resources.limits.cpu | quote }} + memory: {{ .Values.relayNode.resources.limits.memory | quote }} + requests: + cpu: {{ .Values.relayNode.resources.requests.cpu | quote }} + memory: {{ .Values.relayNode.resources.requests.memory | quote }} ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 8Why: Adding resource limits and requests is a best practice that ensures proper resource allocation and prevents resource exhaustion, which is crucial for maintaining the stability and performance of the deployment. | 8 |
Enhancement |
Improve test shuffling reliability using random.sample() instead of random.shuffle()___ **Consider using a more robust method for shuffling tests, such asrandom.sample() , to ensure all tests are included and shuffled effectively.** [tests/SeleniumTests/__init__.py [271-272]](https://github.com/SeleniumHQ/docker-selenium/pull/2453/files#diff-9e40695776a7c3afc35b1bf6d31682fe8ca531271c4a883063aa428b3915387fR271-R272) ```diff mixed_tests.extend(suite) -random.shuffle(mixed_tests) +mixed_tests = random.sample(mixed_tests, len(mixed_tests)) ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 7Why: Using `random.sample()` ensures that all tests are included and shuffled effectively, which can improve the reliability of test execution order. This is a valid enhancement to the test shuffling logic. | 7 |
Add a health check endpoint to the relay node service___ **Consider adding a health check endpoint to the relay node service to improvemonitoring and reliability.** [charts/selenium-grid/templates/relay-node-service.yaml [15-18]](https://github.com/SeleniumHQ/docker-selenium/pull/2453/files#diff-5e418f2b9720be65cd927a7e3263cbe8b5fc2efd3a1e54bcedf19c7375bc8e90R15-R18) ```diff spec: type: {{ .Values.relayNode.service.type }} selector: app: {{ template "seleniumGrid.relayNode.fullname" . }} app.kubernetes.io/instance: {{ .Release.Name }} + ports: + - name: health + port: 8080 + targetPort: 8080 ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 6Why: Adding a health check endpoint can improve monitoring and reliability, but the suggestion lacks specific implementation details and may require additional configuration to be fully effective. | 6 |
π‘ Need additional feedback ? start a PR chat
**Action:** Test Selenium Grid on Kubernetes / Test K8s (v1.28.15, deployment, minikube, v3.13.3, 24.0.9, true, true) |
**Failed stage:** [Test Selenium Grid on Kubernetes v1.28.15 with Autoscaling deployment](https://github.com/SeleniumHQ/docker-selenium/actions/runs/11627518203/job/32380983560) [β] |
**Failed test name:** test_grid_is_up |
**Failure summary:**
The action failed due to multiple issues:test_grid_is_up failed with a TypeError because it attempted to subscript a NoneType object, indicating that the expected data structure was not returned or initialized properly. SSLCertVerificationError due to a self-signed certificate, causing SSL verification to fail. This led to MaxRetryError as the connection could not be established after multiple attempts. |
Relevant error logs:```yaml 1: ##[group]Operating System 2: Ubuntu ... 167: [36;1mfi[0m 168: [36;1m[0m 169: [36;1m# Option: Remove large packages[0m 170: [36;1m# REF: https://github.com/apache/flink/blob/master/tools/azure-pipelines/free_disk_space.sh[0m 171: [36;1m[0m 172: [36;1mif [[ false == 'true' ]]; then[0m 173: [36;1m BEFORE=$(getAvailableSpace)[0m 174: [36;1m [0m 175: [36;1m sudo apt-get remove -y '^aspnetcore-.*' || echo "::warning::The command [sudo apt-get remove -y '^aspnetcore-.*'] failed to complete successfully. Proceeding..."[0m 176: [36;1m sudo apt-get remove -y '^dotnet-.*' --fix-missing || echo "::warning::The command [sudo apt-get remove -y '^dotnet-.*' --fix-missing] failed to complete successfully. Proceeding..."[0m 177: [36;1m sudo apt-get remove -y '^llvm-.*' --fix-missing || echo "::warning::The command [sudo apt-get remove -y '^llvm-.*' --fix-missing] failed to complete successfully. Proceeding..."[0m 178: [36;1m sudo apt-get remove -y 'php.*' --fix-missing || echo "::warning::The command [sudo apt-get remove -y 'php.*' --fix-missing] failed to complete successfully. Proceeding..."[0m 179: [36;1m sudo apt-get remove -y '^mongodb-.*' --fix-missing || echo "::warning::The command [sudo apt-get remove -y '^mongodb-.*' --fix-missing] failed to complete successfully. Proceeding..."[0m 180: [36;1m sudo apt-get remove -y '^mysql-.*' --fix-missing || echo "::warning::The command [sudo apt-get remove -y '^mysql-.*' --fix-missing] failed to complete successfully. Proceeding..."[0m 181: [36;1m sudo apt-get remove -y azure-cli google-chrome-stable firefox powershell mono-devel libgl1-mesa-dri --fix-missing || echo "::warning::The command [sudo apt-get remove -y azure-cli google-chrome-stable firefox powershell mono-devel libgl1-mesa-dri --fix-missing] failed to complete successfully. Proceeding..."[0m 182: [36;1m sudo apt-get remove -y google-cloud-sdk --fix-missing || echo "::debug::The command [sudo apt-get remove -y google-cloud-sdk --fix-missing] failed to complete successfully. Proceeding..."[0m 183: [36;1m sudo apt-get remove -y google-cloud-cli --fix-missing || echo "::debug::The command [sudo apt-get remove -y google-cloud-cli --fix-missing] failed to complete successfully. Proceeding..."[0m 184: [36;1m sudo apt-get autoremove -y || echo "::warning::The command [sudo apt-get autoremove -y] failed to complete successfully. Proceeding..."[0m 185: [36;1m sudo apt-get clean || echo "::warning::The command [sudo apt-get clean] failed to complete successfully. Proceeding..."[0m ... 538: with: 539: timeout_minutes: 10 540: max_attempts: 3 541: command: make setup_dev_env 542: 543: retry_wait_seconds: 10 544: polling_interval_seconds: 1 545: warning_on_retry: true 546: continue_on_error: false ... 1055: go: downloading github.com/shopspring/decimal v1.3.1 1056: go: downloading golang.org/x/crypto v0.21.0 1057: go: downloading github.com/subosito/gotenv v1.4.2 1058: go: downloading github.com/hashicorp/hcl v1.0.0 1059: go: downloading gopkg.in/ini.v1 v1.67.0 1060: go: downloading github.com/magiconair/properties v1.8.7 1061: go: downloading github.com/pelletier/go-toml/v2 v2.0.8 1062: go: downloading github.com/mitchellh/reflectwalk v1.0.2 1063: go: downloading github.com/pkg/errors v0.9.1 ... 1067: helm-docs [flags] 1068: Flags: 1069: -b, --badge-style string badge style to use for charts (default "flat-square") 1070: -c, --chart-search-root string directory to search recursively within for charts (default ".") 1071: -g, --chart-to-generate strings List of charts that will have documentation generated. Comma separated, no space. Empty list - generate for all charts in chart-search-root 1072: -u, --document-dependency-values For charts with dependencies, include the dependency values in the chart values documentation 1073: -y, --documentation-strict-ignore-absent strings A comma separate values which are allowed not to be documented in strict mode (default [service.type,image.repository,image.tag]) 1074: -z, --documentation-strict-ignore-absent-regex strings A comma separate values which are allowed not to be documented in strict mode (default [.*service\.type,.*image\.repository,.*image\.tag]) 1075: -x, --documentation-strict-mode Fail the generation of docs if there are undocumented values 1076: -d, --dry-run don't actually render any markdown files just print to stdout passed 1077: -h, --help help for helm-docs 1078: -i, --ignore-file string The filename to use as an ignore file to exclude chart directories (default ".helmdocsignore") 1079: --ignore-non-descriptions ignore values without a comment, this values will not be included in the README 1080: -l, --log-level string Level of logs that should printed, one of (panic, fatal, error, warning, info, debug, trace) (default "info") ... 1438: VERSION: 4.27.0-SNAPSHOT 1439: BUILD_DATE: 20241101 1440: IMAGE_REGISTRY: artifactory/selenium 1441: AUTHORS: SeleniumHQ 1442: ##[endgroup] 1443: VERSION=4.27.0-SNAPSHOT-20241101 ./tests/charts/make/chart_build.sh 1444: + SET_VERSION=true 1445: + CHART_PATH=charts/selenium-grid 1446: + trap on_failure ERR ... 1512: Downloading jaeger from repo https://jaegertracing.github.io/helm-charts 1513: Downloading kube-prometheus-stack from repo https://prometheus-community.github.io/helm-charts 1514: Deleting outdated charts 1515: Linting chart "selenium-grid => (version: \"0.36.5\", path: \"charts/selenium-grid\")" 1516: Validating /home/runner/work/docker-selenium/docker-selenium/charts/selenium-grid/Chart.yaml... 1517: Validation success! π 1518: Validating maintainers... 1519: ==> Linting charts/selenium-grid 1520: 1 chart(s) linted, 0 chart(s) failed ... 1534: ##[group]Run nick-invision/retry@master 1535: with: 1536: timeout_minutes: 12 1537: max_attempts: 3 1538: retry_wait_seconds: 60 1539: command: NAME=${IMAGE_REGISTRY} VERSION=${BRANCH} BUILD_DATE=${BUILD_DATE} make build 1540: polling_interval_seconds: 1 1541: warning_on_retry: true 1542: continue_on_error: false ... 1572: rm -rf ./Base/configs/node && mkdir -p ./Base/configs/node && cp -r ./charts/selenium-grid/configs/node ./Base/configs 1573: rm -rf ./Base/certs && cp -r ./charts/selenium-grid/certs ./Base 1574: ./Base/certs/gen-cert-helper.sh -d ./Base/certs 1575: Generating 2,048 bit RSA key pair and self-signed certificate (SHA256withRSA) with a validity of 3,650 days 1576: for: CN=SeleniumHQ, OU=Software Freedom Conservancy, O=SeleniumHQ, L=Unknown, ST=Unknown, C=Unknown 1577: [Storing server.jks] 1578: Importing keystore server.jks to tls.p12... 1579: Entry for alias seleniumhq successfully imported. 1580: Import command completed: 1 entries successfully imported, 0 entries failed or cancelled ... 2313: #10 21.57 Downloading https://repo1.maven.org/maven2/io/opentelemetry/opentelemetry-sdk-extension-autoconfigure-spi/1.43.0/opentelemetry-sdk-extension-autoconfigure-spi-1.43.0.pom 2314: #10 21.58 Downloading https://repo1.maven.org/maven2/io/netty/netty-handler/4.1.114.Final/netty-handler-4.1.114.Final.pom 2315: #10 21.58 Downloading https://repo1.maven.org/maven2/io/grpc/grpc-core/1.68.0/grpc-core-1.68.0.pom 2316: #10 21.58 Downloading https://repo1.maven.org/maven2/io/opentelemetry/opentelemetry-exporter-otlp-common/1.43.0/opentelemetry-exporter-otlp-common-1.43.0.pom 2317: #10 21.60 Downloaded https://repo1.maven.org/maven2/io/netty/netty-codec/4.1.114.Final/netty-codec-4.1.114.Final.pom 2318: #10 21.60 Downloading https://repo1.maven.org/maven2/io/opentelemetry/opentelemetry-sdk-trace/1.43.0/opentelemetry-sdk-trace-1.43.0.pom 2319: #10 21.61 Downloaded https://repo1.maven.org/maven2/io/opentelemetry/opentelemetry-sdk-trace/1.43.0/opentelemetry-sdk-trace-1.43.0.pom 2320: #10 21.61 Downloaded https://repo1.maven.org/maven2/io/netty/netty-handler/4.1.114.Final/netty-handler-4.1.114.Final.pom 2321: #10 21.61 Downloading https://repo1.maven.org/maven2/com/google/errorprone/error_prone_annotations/2.28.0/error_prone_annotations-2.28.0.pom 2322: #10 21.61 Downloaded https://repo1.maven.org/maven2/io/opentelemetry/opentelemetry-exporter-otlp-common/1.43.0/opentelemetry-exporter-otlp-common-1.43.0.pom 2323: #10 21.61 Downloading https://repo1.maven.org/maven2/io/opentelemetry/opentelemetry-sdk-metrics/1.43.0/opentelemetry-sdk-metrics-1.43.0.pom 2324: #10 21.61 Downloading https://repo1.maven.org/maven2/io/netty/netty-handler-proxy/4.1.110.Final/netty-handler-proxy-4.1.110.Final.pom 2325: #10 21.61 Downloaded https://repo1.maven.org/maven2/io/grpc/grpc-core/1.68.0/grpc-core-1.68.0.pom 2326: #10 21.61 Downloaded https://repo1.maven.org/maven2/com/google/errorprone/error_prone_annotations/2.28.0/error_prone_annotations-2.28.0.pom ... 2344: #10 21.66 Downloading https://repo1.maven.org/maven2/io/netty/netty-codec-http2/4.1.110.Final/netty-codec-http2-4.1.110.Final.pom 2345: #10 21.66 Downloading https://repo1.maven.org/maven2/io/netty/netty-common/4.1.114.Final/netty-common-4.1.114.Final.pom 2346: #10 21.66 Downloaded https://repo1.maven.org/maven2/io/grpc/grpc-util/1.68.0/grpc-util-1.68.0.pom 2347: #10 21.67 Downloaded https://repo1.maven.org/maven2/io/netty/netty-common/4.1.114.Final/netty-common-4.1.114.Final.pom 2348: #10 21.67 Downloaded https://repo1.maven.org/maven2/io/netty/netty-codec-http2/4.1.110.Final/netty-codec-http2-4.1.110.Final.pom 2349: #10 21.68 Downloaded https://repo1.maven.org/maven2/io/netty/netty-transport-native-unix-common/4.1.110.Final/netty-transport-native-unix-common-4.1.110.Final.pom 2350: #10 21.68 Downloaded https://repo1.maven.org/maven2/io/opentelemetry/opentelemetry-sdk-logs/1.43.0/opentelemetry-sdk-logs-1.43.0.pom 2351: #10 21.71 Downloading https://repo1.maven.org/maven2/io/netty/netty-parent/4.1.110.Final/netty-parent-4.1.110.Final.pom 2352: #10 21.71 Downloading https://repo1.maven.org/maven2/com/google/errorprone/error_prone_parent/2.28.0/error_prone_parent-2.28.0.pom 2353: #10 21.71 Downloading https://repo1.maven.org/maven2/com/google/guava/guava-parent/33.2.1-android/guava-parent-33.2.1-android.pom 2354: #10 21.72 Downloaded https://repo1.maven.org/maven2/com/google/errorprone/error_prone_parent/2.28.0/error_prone_parent-2.28.0.pom ... 2505: #10 22.43 Downloaded https://repo1.maven.org/maven2/com/google/guava/listenablefuture/9999.0-empty-to-avoid-conflict-with-guava/listenablefuture-9999.0-empty-to-avoid-conflict-with-guava.jar 2506: #10 22.43 Downloaded https://repo1.maven.org/maven2/io/netty/netty-handler-proxy/4.1.110.Final/netty-handler-proxy-4.1.110.Final.jar 2507: #10 22.43 Downloading https://repo1.maven.org/maven2/io/opentelemetry/opentelemetry-context/1.43.0/opentelemetry-context-1.43.0.jar 2508: #10 22.44 Downloaded https://repo1.maven.org/maven2/io/opentelemetry/opentelemetry-exporter-sender-okhttp/1.43.0/opentelemetry-exporter-sender-okhttp-1.43.0.jar 2509: #10 22.44 Downloaded https://repo1.maven.org/maven2/io/opentelemetry/opentelemetry-exporter-otlp/1.43.0/opentelemetry-exporter-otlp-1.43.0.jar 2510: #10 22.44 Downloaded https://repo1.maven.org/maven2/io/opentelemetry/opentelemetry-context/1.43.0/opentelemetry-context-1.43.0.jar 2511: #10 22.44 Downloaded https://repo1.maven.org/maven2/com/google/android/annotations/4.1.1.4/annotations-4.1.1.4.jar 2512: #10 22.45 Downloading https://repo1.maven.org/maven2/org/jetbrains/kotlin/kotlin-stdlib/1.9.10/kotlin-stdlib-1.9.10.jar 2513: #10 22.46 Downloading https://repo1.maven.org/maven2/com/google/errorprone/error_prone_annotations/2.28.0/error_prone_annotations-2.28.0.jar 2514: #10 22.46 Downloading https://repo1.maven.org/maven2/io/netty/netty-codec/4.1.114.Final/netty-codec-4.1.114.Final.jar 2515: #10 22.46 Downloading https://repo1.maven.org/maven2/io/netty/netty-buffer/4.1.114.Final/netty-buffer-4.1.114.Final.jar 2516: #10 22.46 Downloading https://repo1.maven.org/maven2/org/checkerframework/checker-qual/3.42.0/checker-qual-3.42.0.jar 2517: #10 22.47 Downloaded https://repo1.maven.org/maven2/com/google/errorprone/error_prone_annotations/2.28.0/error_prone_annotations-2.28.0.jar ... 2531: #14 DONE 0.0s 2532: #15 [stage-0 7/8] COPY --chown=1200:1201 certs/tls.crt certs/tls.key certs/server.jks certs/server.pass /opt/selenium/secrets/ 2533: #15 DONE 0.0s 2534: #16 [stage-0 8/8] RUN /opt/bin/add-jks-helper.sh -d /opt/selenium/secrets && /opt/bin/add-cert-helper.sh -d /opt/selenium/secrets TCu,Cu,Tu 2535: #16 0.153 seluser is running cert script! 2536: #16 0.528 Processing /opt/selenium/secrets/server.jks 2537: #16 0.814 Certificate stored in file 2538: #16 0.973 Warning: use -cacerts option to access cacerts keystore 2539: #16 1.079 keytool error: java.lang.Exception: Alias |
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
Fixes #2111 By default, it is disabled User can replace/add images via initContainers or sidecars, also can add extra env vars, volumes, etc.
Motivation and Context
Types of changes
Checklist
PR Type
Enhancement, Tests
Description
Changes walkthrough π
5 files
relay-node-deployment.yaml
Add deployment template for relay nodes
charts/selenium-grid/templates/relay-node-deployment.yaml
relay-node-hpa.yaml
Add autoscaling configuration for relay nodes
charts/selenium-grid/templates/relay-node-hpa.yaml
relay-node-scaledjobs.yaml
Add job scaling configuration for relay nodes
charts/selenium-grid/templates/relay-node-scaledjobs.yaml
relay-node-service.yaml
Add service configuration for relay nodes
charts/selenium-grid/templates/relay-node-service.yaml
_nameHelpers.tpl
Add helper for relay node naming
charts/selenium-grid/templates/_nameHelpers.tpl - Added a helper function for relay node fullname.
1 files
CONFIGURATION.md
Document relay node configuration options
charts/selenium-grid/CONFIGURATION.md
1 files
values.yaml
Define relay node configuration settings
charts/selenium-grid/values.yaml
2 files
__init__.py
Enhance test execution with random order
tests/SeleniumTests/__init__.py
docker-compose-v3-test-node-relay.yml
Update docker-compose for relay node testing
tests/docker-compose-v3-test-node-relay.yml