Closed VietND96 closed 1 month ago
Here are some key observations to aid the review process:
β±οΈ Estimated effort to review: 3 π΅π΅π΅βͺβͺ |
π§ͺ PR contains tests |
π No security concerns identified |
β‘ Recommended focus areas for review Potential Bug The change in video recorder configuration might cause issues if not all expected fields are present in the node-specific videoRecorder object. Ensure proper fallback or default values are in place. Configuration Complexity The use of `mergeOverwrite` for video recorder settings may lead to unexpected behavior if not carefully managed. Ensure that all possible configuration scenarios are tested. Documentation Needed The newly added `videoRecorder` configuration for each node type lacks detailed documentation. Consider adding examples and explanations for proper usage. |
Explore these optional code suggestions:
Category | Suggestion | Score |
Enhancement |
Ensure backwards compatibility by adding fallback to global video recorder settings___ **The condition for enabling the recorder is now tied to the node-specificconfiguration. Consider adding a fallback to the global video recorder setting to ensure backwards compatibility.** [charts/selenium-grid/templates/_helpers.tpl [481-484]](https://github.com/SeleniumHQ/docker-selenium/pull/2445/files#diff-a57e8d3d1ad8ed854bf7e00ac004560f3dfe6d1178d5c5d45e455f8eaeb53361R481-R484) ```diff -{{- if .recorder.enabled }} - - name: {{ .recorder.name }} - image: {{ printf "%s/%s:%s" $videoImageRegistry .recorder.imageName $videoImageTag }} - imagePullPolicy: {{ .recorder.imagePullPolicy }} +{{- if or .recorder.enabled $.Values.videoRecorder.enabled }} + - name: {{ .recorder.name | default $.Values.videoRecorder.name }} + image: {{ printf "%s/%s:%s" $videoImageRegistry (.recorder.imageName | default $.Values.videoRecorder.imageName) $videoImageTag }} + imagePullPolicy: {{ .recorder.imagePullPolicy | default $.Values.videoRecorder.imagePullPolicy }} ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 8Why: Adding a fallback to global settings ensures backwards compatibility, which is crucial for maintaining existing functionality while introducing new features. This suggestion addresses a significant aspect of the code. | 8 |
Improve variable naming for better code readability and understanding___ **Consider using a more descriptive variable name instead ofdoc to improve code readability. For example, node_config or selenium_node would be more informative.**
[tests/charts/templates/test.py [211-213]](https://github.com/SeleniumHQ/docker-selenium/pull/2445/files#diff-313e81a96e34d08dcd821e7a1223f59ffce689433658ae3999a26fc7f6dcb343R211-R213)
```diff
-if doc['metadata']['name'] == '{0}selenium-edge-node'.format(RELEASE_NAME):
+if node_config['metadata']['name'] == '{0}selenium-edge-node'.format(RELEASE_NAME):
self.assertTrue(uploader_container is None, "Video uploader should be disabled in Edge node config")
continue
```
- [ ] **Apply this suggestion**
Suggestion importance[1-10]: 5Why: The suggestion to use a more descriptive variable name improves code readability and understanding, which is beneficial for maintainability. However, it does not address a critical issue, so the impact is moderate. | 5 | |
Best practice |
Standardize variable naming conventions for improved code consistency___ **Consider using a more consistent naming convention for variables. The PR introducesrecorder and uploader , while the existing code uses videoRecorder . Align the naming to improve consistency and readability.** [charts/selenium-grid/templates/_helpers.tpl [280-281]](https://github.com/SeleniumHQ/docker-selenium/pull/2445/files#diff-a57e8d3d1ad8ed854bf7e00ac004560f3dfe6d1178d5c5d45e455f8eaeb53361R280-R281) ```diff -{{- $videoImageRegistry := default $.Values.global.seleniumGrid.imageRegistry .recorder.imageRegistry -}} -{{- $videoImageTag := default $.Values.global.seleniumGrid.videoImageTag .recorder.imageTag -}} +{{- $videoRecorderImageRegistry := default $.Values.global.seleniumGrid.imageRegistry .videoRecorder.imageRegistry -}} +{{- $videoRecorderImageTag := default $.Values.global.seleniumGrid.videoImageTag .videoRecorder.imageTag -}} ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 6Why: Aligning variable naming conventions enhances code consistency and readability, which is a good practice. This suggestion is relevant and improves the overall quality of the code. | 6 |
π‘ Need additional feedback ? start a PR chat
**Action:** Test Seleium Grid on Docker / Test Docker Selenium (test_node_relay, false, false, false) |
**Failed stage:** [Run Docker Compose to test_node_relay](https://github.com/SeleniumHQ/docker-selenium/actions/runs/11540630607/job/32121682867) [β] |
**Failed test name:** "" |
**Failure summary:**
The action failed due to multiple issues:keytool command encountered an error: java.lang.Exception: Alias . This indicates a problem with the certificate alias not being found during the certificate processing step. systemd-network and systemd-journal , indicating potential misconfigurations in the systemd setup.OpenTelemetry. The error message Failed to connect to localhost/[0:0:0:0:0:0:0:1]:4317 suggests that the service expected to be running on this port was not available, causing the span export process to fail. |
Relevant error logs:```yaml 1: ##[group]Operating System 2: Ubuntu ... 159: [36;1mfi[0m 160: [36;1m[0m 161: [36;1m# Option: Remove large packages[0m 162: [36;1m# REF: https://github.com/apache/flink/blob/master/tools/azure-pipelines/free_disk_space.sh[0m 163: [36;1m[0m 164: [36;1mif [[ false == 'true' ]]; then[0m 165: [36;1m BEFORE=$(getAvailableSpace)[0m 166: [36;1m [0m 167: [36;1m sudo apt-get remove -y '^aspnetcore-.*' || echo "::warning::The command [sudo apt-get remove -y '^aspnetcore-.*'] failed to complete successfully. Proceeding..."[0m 168: [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 169: [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 170: [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 171: [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 172: [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 173: [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 174: [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 175: [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 176: [36;1m sudo apt-get autoremove -y || echo "::warning::The command [sudo apt-get autoremove -y] failed to complete successfully. Proceeding..."[0m 177: [36;1m sudo apt-get clean || echo "::warning::The command [sudo apt-get clean] failed to complete successfully. Proceeding..."[0m ... 570: with: 571: timeout_minutes: 10 572: max_attempts: 3 573: command: make setup_dev_env 574: 575: retry_wait_seconds: 10 576: polling_interval_seconds: 1 577: warning_on_retry: true 578: continue_on_error: false ... 1077: go: downloading golang.org/x/crypto v0.21.0 1078: go: downloading golang.org/x/text v0.14.0 1079: go: downloading github.com/subosito/gotenv v1.4.2 1080: go: downloading github.com/hashicorp/hcl v1.0.0 1081: go: downloading gopkg.in/ini.v1 v1.67.0 1082: go: downloading github.com/magiconair/properties v1.8.7 1083: go: downloading github.com/pelletier/go-toml/v2 v2.0.8 1084: go: downloading github.com/mitchellh/reflectwalk v1.0.2 1085: go: downloading github.com/pkg/errors v0.9.1 ... 1088: helm-docs [flags] 1089: Flags: 1090: -b, --badge-style string badge style to use for charts (default "flat-square") 1091: -c, --chart-search-root string directory to search recursively within for charts (default ".") 1092: -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 1093: -u, --document-dependency-values For charts with dependencies, include the dependency values in the chart values documentation 1094: -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]) 1095: -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]) 1096: -x, --documentation-strict-mode Fail the generation of docs if there are undocumented values 1097: -d, --dry-run don't actually render any markdown files just print to stdout passed 1098: -h, --help help for helm-docs 1099: -i, --ignore-file string The filename to use as an ignore file to exclude chart directories (default ".helmdocsignore") 1100: --ignore-non-descriptions ignore values without a comment, this values will not be included in the README 1101: -l, --log-level string Level of logs that should printed, one of (panic, fatal, error, warning, info, debug, trace) (default "info") ... 1371: retry_wait_seconds: 60 1372: command: VERSION=${BRANCH} BUILD_DATE=${BUILD_DATE} make hub 1373: VERSION=${BRANCH} BUILD_DATE=${BUILD_DATE} make chrome 1374: VERSION=${BRANCH} BUILD_DATE=${BUILD_DATE} make firefox 1375: VERSION=${BRANCH} BUILD_DATE=${BUILD_DATE} make edge 1376: 1377: polling_interval_seconds: 1 1378: warning_on_retry: true 1379: continue_on_error: false ... 1394: rm -rf ./Base/configs/node && mkdir -p ./Base/configs/node && cp -r ./charts/selenium-grid/configs/node ./Base/configs 1395: rm -rf ./Base/certs && cp -r ./charts/selenium-grid/certs ./Base 1396: ./Base/certs/gen-cert-helper.sh -d ./Base/certs 1397: Generating 2,048 bit RSA key pair and self-signed certificate (SHA256withRSA) with a validity of 3,650 days 1398: for: CN=SeleniumHQ, OU=Software Freedom Conservancy, O=SeleniumHQ, L=Unknown, ST=Unknown, C=Unknown 1399: [Storing server.jks] 1400: Importing keystore server.jks to tls.p12... 1401: Entry for alias seleniumhq successfully imported. 1402: Import command completed: 1 entries successfully imported, 0 entries failed or cancelled ... 2205: #10 22.12 Downloading https://repo1.maven.org/maven2/io/grpc/grpc-core/1.66.0/grpc-core-1.66.0.pom 2206: #10 22.13 Downloaded https://repo1.maven.org/maven2/com/google/guava/guava/33.2.1-android/guava-33.2.1-android.pom 2207: #10 22.13 Downloading https://repo1.maven.org/maven2/io/netty/netty-common/4.1.113.Final/netty-common-4.1.113.Final.pom 2208: #10 22.14 Downloaded https://repo1.maven.org/maven2/io/opentelemetry/opentelemetry-sdk-metrics/1.42.1/opentelemetry-sdk-metrics-1.42.1.pom 2209: #10 22.14 Downloading https://repo1.maven.org/maven2/io/netty/netty-transport-native-unix-common/4.1.100.Final/netty-transport-native-unix-common-4.1.100.Final.pom 2210: #10 22.15 Downloaded https://repo1.maven.org/maven2/io/netty/netty-buffer/4.1.113.Final/netty-buffer-4.1.113.Final.pom 2211: #10 22.15 Downloaded https://repo1.maven.org/maven2/io/opentelemetry/opentelemetry-sdk-logs/1.42.1/opentelemetry-sdk-logs-1.42.1.pom 2212: #10 22.15 Downloading https://repo1.maven.org/maven2/io/opentelemetry/opentelemetry-exporter-sender-okhttp/1.42.1/opentelemetry-exporter-sender-okhttp-1.42.1.pom 2213: #10 22.15 Downloading https://repo1.maven.org/maven2/com/google/errorprone/error_prone_annotations/2.28.0/error_prone_annotations-2.28.0.pom 2214: #10 22.15 Downloaded https://repo1.maven.org/maven2/io/netty/netty-transport-native-unix-common/4.1.100.Final/netty-transport-native-unix-common-4.1.100.Final.pom 2215: #10 22.16 Downloading https://repo1.maven.org/maven2/io/perfmark/perfmark-api/0.27.0/perfmark-api-0.27.0.pom 2216: #10 22.16 Downloaded https://repo1.maven.org/maven2/io/netty/netty-common/4.1.113.Final/netty-common-4.1.113.Final.pom 2217: #10 22.16 Downloaded https://repo1.maven.org/maven2/io/opentelemetry/opentelemetry-exporter-sender-okhttp/1.42.1/opentelemetry-exporter-sender-okhttp-1.42.1.pom 2218: #10 22.16 Downloading https://repo1.maven.org/maven2/io/netty/netty-handler/4.1.113.Final/netty-handler-4.1.113.Final.pom 2219: #10 22.16 Downloaded https://repo1.maven.org/maven2/io/netty/netty-transport/4.1.113.Final/netty-transport-4.1.113.Final.pom 2220: #10 22.16 Downloaded https://repo1.maven.org/maven2/io/grpc/grpc-core/1.66.0/grpc-core-1.66.0.pom 2221: #10 22.17 Downloading https://repo1.maven.org/maven2/io/netty/netty-codec/4.1.113.Final/netty-codec-4.1.113.Final.pom 2222: #10 22.17 Downloaded https://repo1.maven.org/maven2/com/google/errorprone/error_prone_annotations/2.28.0/error_prone_annotations-2.28.0.pom ... 2232: #10 22.21 Downloading https://repo1.maven.org/maven2/io/opentelemetry/opentelemetry-sdk-extension-autoconfigure-spi/1.42.1/opentelemetry-sdk-extension-autoconfigure-spi-1.42.1.pom 2233: #10 22.21 Downloaded https://repo1.maven.org/maven2/io/opentelemetry/opentelemetry-exporter-otlp-common/1.42.1/opentelemetry-exporter-otlp-common-1.42.1.pom 2234: #10 22.22 Downloaded https://repo1.maven.org/maven2/io/netty/netty-codec-http2/4.1.100.Final/netty-codec-http2-4.1.100.Final.pom 2235: #10 22.22 Downloaded https://repo1.maven.org/maven2/io/netty/netty-handler-proxy/4.1.100.Final/netty-handler-proxy-4.1.100.Final.pom 2236: #10 22.23 Downloaded https://repo1.maven.org/maven2/io/opentelemetry/opentelemetry-sdk-trace/1.42.1/opentelemetry-sdk-trace-1.42.1.pom 2237: #10 22.23 Downloaded https://repo1.maven.org/maven2/io/opentelemetry/opentelemetry-sdk-extension-autoconfigure-spi/1.42.1/opentelemetry-sdk-extension-autoconfigure-spi-1.42.1.pom 2238: #10 22.25 Downloading https://repo1.maven.org/maven2/io/netty/netty-parent/4.1.100.Final/netty-parent-4.1.100.Final.pom 2239: #10 22.25 Downloading https://repo1.maven.org/maven2/com/google/guava/guava-parent/33.2.1-android/guava-parent-33.2.1-android.pom 2240: #10 22.25 Downloading https://repo1.maven.org/maven2/com/google/errorprone/error_prone_parent/2.28.0/error_prone_parent-2.28.0.pom 2241: #10 22.26 Downloaded https://repo1.maven.org/maven2/com/google/guava/guava-parent/33.2.1-android/guava-parent-33.2.1-android.pom 2242: #10 22.26 Downloaded https://repo1.maven.org/maven2/com/google/errorprone/error_prone_parent/2.28.0/error_prone_parent-2.28.0.pom ... 2337: #10 22.82 Downloading https://repo1.maven.org/maven2/io/grpc/grpc-netty/1.66.0/grpc-netty-1.66.0.jar 2338: #10 22.83 Downloaded https://repo1.maven.org/maven2/io/netty/netty-codec-socks/4.1.100.Final/netty-codec-socks-4.1.100.Final.jar 2339: #10 22.83 Downloading https://repo1.maven.org/maven2/org/checkerframework/checker-qual/3.42.0/checker-qual-3.42.0.jar 2340: #10 22.83 Downloaded https://repo1.maven.org/maven2/org/codehaus/mojo/animal-sniffer-annotations/1.24/animal-sniffer-annotations-1.24.jar 2341: #10 22.83 Downloading https://repo1.maven.org/maven2/io/grpc/grpc-util/1.66.0/grpc-util-1.66.0.jar 2342: #10 22.84 Downloaded https://repo1.maven.org/maven2/io/grpc/grpc-util/1.66.0/grpc-util-1.66.0.jar 2343: #10 22.84 Downloading https://repo1.maven.org/maven2/io/netty/netty-codec-http/4.1.113.Final/netty-codec-http-4.1.113.Final.jar 2344: #10 22.84 Downloaded https://repo1.maven.org/maven2/io/grpc/grpc-netty/1.66.0/grpc-netty-1.66.0.jar 2345: #10 22.84 Downloading https://repo1.maven.org/maven2/com/google/errorprone/error_prone_annotations/2.28.0/error_prone_annotations-2.28.0.jar ... 2350: #10 22.85 Downloaded https://repo1.maven.org/maven2/io/netty/netty-common/4.1.113.Final/netty-common-4.1.113.Final.jar 2351: #10 22.86 Downloading https://repo1.maven.org/maven2/com/google/code/gson/gson/2.11.0/gson-2.11.0.jar 2352: #10 22.86 Downloaded https://repo1.maven.org/maven2/com/squareup/okio/okio-jvm/3.6.0/okio-jvm-3.6.0.jar 2353: #10 22.86 Downloading https://repo1.maven.org/maven2/io/opentelemetry/opentelemetry-context/1.42.1/opentelemetry-context-1.42.1.jar 2354: #10 22.86 Downloaded https://repo1.maven.org/maven2/io/opentelemetry/opentelemetry-sdk-trace/1.42.1/opentelemetry-sdk-trace-1.42.1.jar 2355: #10 22.86 Downloading https://repo1.maven.org/maven2/org/jetbrains/kotlin/kotlin-stdlib-common/1.9.10/kotlin-stdlib-common-1.9.10.jar 2356: #10 22.87 Downloaded https://repo1.maven.org/maven2/io/opentelemetry/opentelemetry-sdk/1.42.1/opentelemetry-sdk-1.42.1.jar 2357: #10 22.87 Downloading https://repo1.maven.org/maven2/com/google/android/annotations/4.1.1.4/annotations-4.1.1.4.jar 2358: #10 22.87 Downloaded https://repo1.maven.org/maven2/com/google/errorprone/error_prone_annotations/2.28.0/error_prone_annotations-2.28.0.jar ... 2418: #14 DONE 0.0s 2419: #15 [stage-0 7/8] COPY --chown=1200:1201 certs/tls.crt certs/tls.key certs/server.jks certs/server.pass /opt/selenium/secrets/ 2420: #15 DONE 0.0s 2421: #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 2422: #16 0.132 seluser is running cert script! 2423: #16 0.504 Processing /opt/selenium/secrets/server.jks 2424: #16 0.794 Certificate stored in file 2425: #16 0.937 Warning: use -cacerts option to access cacerts keystore 2426: #16 1.041 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
In the Helm chart, the config key
videoRecorder
holds all configs needed for the video recorder in all nodes. Now, in each Node, it has the capability to overwrite the common videoRecorder configs.Motivation and Context
Types of changes
Checklist
PR Type
Enhancement, Tests
Description
mergeOverwrite
.Changes walkthrough π
3 files
test.py
Add test for Edge node video uploader configuration
tests/charts/templates/test.py
dummy.yaml
Update dummy configuration for Edge node video uploader
tests/charts/templates/render/dummy.yaml - Disabled video uploader for Edge node in dummy configuration.
dummy_solution.yaml
Update dummy solution for Edge node video uploader
tests/charts/templates/render/dummy_solution.yaml
7 files
_helpers.tpl
Allow node-specific video recorder configuration overrides
charts/selenium-grid/templates/_helpers.tpl
videoRecorder
torecorder
.chrome-node-deployment.yaml
Enable Chrome node-specific video recorder configuration
charts/selenium-grid/templates/chrome-node-deployment.yaml
mergeOverwrite
for video recorder settings.chrome-node-scaledjobs.yaml
Enable Chrome scaled jobs node-specific video recorder configuration
charts/selenium-grid/templates/chrome-node-scaledjobs.yaml
jobs.
mergeOverwrite
for video recorder settings.edge-node-deployment.yaml
Enable Edge node-specific video recorder configuration
charts/selenium-grid/templates/edge-node-deployment.yaml
mergeOverwrite
for video recorder settings.edge-node-scaledjob.yaml
Enable Edge scaled jobs node-specific video recorder configuration
charts/selenium-grid/templates/edge-node-scaledjob.yaml
jobs.
mergeOverwrite
for video recorder settings.firefox-node-deployment.yaml
Enable Firefox node-specific video recorder configuration
charts/selenium-grid/templates/firefox-node-deployment.yaml
mergeOverwrite
for video recorder settings.firefox-node-scaledjob.yaml
Enable Firefox scaled jobs node-specific video recorder configuration
charts/selenium-grid/templates/firefox-node-scaledjob.yaml
jobs.
mergeOverwrite
for video recorder settings.1 files
CONFIGURATION.md
Document node-specific video recorder configuration
charts/selenium-grid/CONFIGURATION.md
videoRecorder
in node configurations.1 files
values.yaml
Add node-specific video recorder configuration in values
charts/selenium-grid/values.yaml
videoRecorder
configuration for each node type.