Closed VietND96 closed 1 week ago
Here are some key observations to aid the review process:
**๐ซ Ticket compliance analysis ๐ถ** **[2464](https://github.com/SeleniumHQ/docker-selenium/issues/2464) - Partially compliant** Compliant requirements: * Fix scaling issue by changing metricType to 'Value' instead of 'AverageValue' in KEDA configuration Non-compliant requirements: * No automated tests added to verify the scaling behavior |
โฑ๏ธ Estimated effort to review: 2 ๐ต๐ตโชโชโช |
๐งช No relevant tests |
๐ Security concerns Authentication Credentials Handling: The PR modifies how authentication credentials are handled in tests/SmokeTests/__init__.py, changing from empty string defaults to None. While this is generally safer, it should be verified that this doesn't break existing authentication flows or expose the system to unauthorized access. |
โก Recommended focus areas for review Security Configuration The change from empty string defaults to None for authentication credentials might affect existing deployments that rely on empty string behavior Stability Risk Changing Debian source from 'sid' to 'stable' might affect chromium version compatibility |
Explore these optional code suggestions:
Category | Suggestion | Score |
Possible issue |
Handle missing authentication credentials gracefully to prevent potential runtime errors___ **Add error handling for the case when authentication credentials are required but notprovided, as SELENIUM_GRID_USERNAME and SELENIUM_GRID_PASSWORD could be None.**
[tests/SmokeTests/__init__.py [59]](https://github.com/SeleniumHQ/docker-selenium/pull/2465/files#diff-6520381fba53133fa2a295057467ca85aee647dcc3a6388bf66de3b5b1a1a246R59-R59)
```diff
-response = requests.get(grid_url_status, verify=cert_path, auth=HTTPBasicAuth(SELENIUM_GRID_USERNAME, SELENIUM_GRID_PASSWORD))
+auth = HTTPBasicAuth(SELENIUM_GRID_USERNAME, SELENIUM_GRID_PASSWORD) if SELENIUM_GRID_USERNAME and SELENIUM_GRID_PASSWORD else None
+response = requests.get(grid_url_status, verify=cert_path, auth=auth)
```
- [ ] **Apply this suggestion**
Suggestion importance[1-10]: 8Why: The suggestion addresses a potential runtime error by gracefully handling cases where authentication credentials are None, which is now the default value. This is particularly important as the PR changed the default values from empty strings to None. | 8 |
Validate certificate file existence before using it to prevent potential runtime errors___ **Validate the certificate path before using it in requests.get() to avoid runtimeerrors if the certificate file is not accessible.** [tests/SmokeTests/__init__.py [20]](https://github.com/SeleniumHQ/docker-selenium/pull/2465/files#diff-6520381fba53133fa2a295057467ca85aee647dcc3a6388bf66de3b5b1a1a246R20-R20) ```diff -os.environ['REQUESTS_CA_BUNDLE'] = CHART_CERT_PATH +if CHART_CERT_PATH and os.path.isfile(CHART_CERT_PATH): + os.environ['REQUESTS_CA_BUNDLE'] = CHART_CERT_PATH ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 7Why: This suggestion prevents potential runtime errors by validating the certificate file's existence before setting it as an environment variable, which is important since CHART_CERT_PATH can now be None in the updated code. | 7 | |
Best practice |
Specify architecture in package repository configuration for more precise dependency management___ **Add version pinning for the Debian packages to ensure reproducible builds andprevent unexpected updates.** [NodeChromium/Dockerfile [12]](https://github.com/SeleniumHQ/docker-selenium/pull/2465/files#diff-ddc70cdbe415111d77d5eadd39b842ca541bd0f0a9ecb04fceb7597f2f09d598R12-R12) ```diff -RUN echo "deb ${CHROMIUM_DEB_SITE}/ stable main" >> /etc/apt/sources.list \ +RUN echo "deb [arch=amd64] ${CHROMIUM_DEB_SITE}/ stable main" >> /etc/apt/sources.list \ ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 4Why: While adding architecture specification provides more explicit package management, it's a minor improvement since the default architecture would likely work correctly in most cases. The change from 'sid' to 'stable' is already handling the main stability concern. | 4 |
๐ก Need additional feedback ? start a PR chat
**Action:** Rerun workflow when failure |
**Failed stage:** [Authenticate GitHub CLI for PR](https://github.com/SeleniumHQ/docker-selenium/actions/runs/11828529104/job/32961456973) [โ] |
**Failure summary:**
The action failed due to an error in the authentication process with GitHub CLI:read:org . |
Relevant error logs:```yaml 1: ##[group]Operating System 2: Ubuntu ... 28: SecurityEvents: write 29: Statuses: write 30: ##[endgroup] 31: Secret source: Actions 32: Prepare workflow directory 33: Prepare all required actions 34: Getting action download info 35: Download action repository 'actions/checkout@main' (SHA:3b9b8c884f6b4bb4d5be2779c26374abadae0871) 36: Complete job name: Rerun workflow when failure ... 48: show-progress: true 49: lfs: false 50: submodules: false 51: set-safe-directory: true 52: env: 53: GH_CLI_TOKEN: *** 54: GH_CLI_TOKEN_PR: *** 55: RUN_ID: 11828529104 56: RERUN_FAILED_ONLY: true ... 119: ##[group]Run sudo apt update 120: [36;1msudo apt update[0m 121: [36;1msudo apt install gh[0m 122: shell: /usr/bin/bash -e {0} 123: env: 124: GH_CLI_TOKEN: *** 125: GH_CLI_TOKEN_PR: *** 126: RUN_ID: 11828529104 127: RERUN_FAILED_ONLY: true ... 165: 0 upgraded, 0 newly installed, 0 to remove and 55 not upgraded. 166: ##[group]Run echo "$GH_CLI_TOKEN_PR" | gh auth login --with-token 167: [36;1mecho "$GH_CLI_TOKEN_PR" | gh auth login --with-token[0m 168: shell: /usr/bin/bash -e {0} 169: env: 170: GH_CLI_TOKEN: *** 171: GH_CLI_TOKEN_PR: *** 172: RUN_ID: 11828529104 173: RERUN_FAILED_ONLY: true 174: RUN_ATTEMPT: 1 175: ##[endgroup] 176: error validating token: missing required scope 'read:org' 177: ##[error]Process completed with exit code 1. ``` |
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 #2464 - Node autoscaling with scale type Deployment Following KEDA docs - https://keda.sh/docs/2.16/reference/scaledobject-spec/#triggers, adjust configs in default chart values
useCachedMetrics: true
- default isfalse
in ScaledObjectmetricType: Value
- default isAverageValue
in ScaledObjectMotivation and Context
Types of changes
Checklist
PR Type
Bug fix, Enhancement
Description
useCachedMetrics
andmetricType
options, enhancing the scaling logic.NodeChromium
Dockerfile fromsid
tostable
for better stability.None
and adding HTTP basic authentication for requests.Changes walkthrough ๐
__init__.py
Update environment variables and add authentication for requests
tests/SmokeTests/__init__.py
None
.CHART_CERT_PATH
environment variable.requests.get
to include HTTP basic authentication._helpers.tpl
Enhance autoscaling triggers with additional configurations
charts/selenium-grid/templates/_helpers.tpl
useCachedMetrics
andmetricType
to autoscaling triggers.nodeMaxSessions
is set if not provided.Dockerfile
Update Debian source to stable in Dockerfile
NodeChromium/Dockerfile - Changed Debian source from `sid` to `stable`.
CONFIGURATION.md
Add documentation for new autoscaling options
charts/selenium-grid/CONFIGURATION.md - Documented `useCachedMetrics` and `metricType` options.
values.yaml
Add new autoscaling configuration options
charts/selenium-grid/values.yaml
useCachedMetrics
andmetricType
to autoscaling configuration.