[ci] perform test selection outside container #58527
Conversation
previously it was actually using 0.4.0, which is set up by the grpc repo. the declaration in the workspace file was being shadowed.. Signed-off-by: Lonnie Liu <[email protected]>
| ], | ||
| ) | ||
|
|
||
| http_archive( |
There was a problem hiding this comment.
this seems to be the highest version that will not break anything else..
There was a problem hiding this comment.
Code Review
This pull request correctly fixes a dependency shadowing issue with rules_python in the Bazel WORKSPACE. By moving the http_archive declaration to the top of the file, you ensure that the intended version (0.25.0) is used, rather than a version from a transitive dependency, which is a great fix for build determinism. I've added one suggestion to improve the resilience of the dependency download by using mirror URLs. Overall, this is a good improvement to the build configuration.
| http_archive( | ||
| name = "rules_python", | ||
| sha256 = "5868e73107a8e85d8f323806e60cad7283f34b32163ea6ff1020cf27abef6036", | ||
| strip_prefix = "rules_python-0.25.0", | ||
| url = "https://github.com/bazelbuild/rules_python/releases/download/0.25.0/rules_python-0.25.0.tar.gz", | ||
| ) |
There was a problem hiding this comment.
For better resilience against network issues or source downtime, it's a good practice to provide mirror URLs for http_archive. You can replace url with urls and include mirror.bazel.build as a fallback.
| http_archive( | |
| name = "rules_python", | |
| sha256 = "5868e73107a8e85d8f323806e60cad7283f34b32163ea6ff1020cf27abef6036", | |
| strip_prefix = "rules_python-0.25.0", | |
| url = "https://github.com/bazelbuild/rules_python/releases/download/0.25.0/rules_python-0.25.0.tar.gz", | |
| ) | |
| http_archive( | |
| name = "rules_python", | |
| sha256 = "5868e73107a8e85d8f323806e60cad7283f34b32163ea6ff1020cf27abef6036", | |
| strip_prefix = "rules_python-0.25.0", | |
| urls = [ | |
| "https://mirror.bazel.build/github.com/bazelbuild/rules_python/releases/download/0.25.0/rules_python-0.25.0.tar.gz", | |
| "https://github.com/bazelbuild/rules_python/releases/download/0.25.0/rules_python-0.25.0.tar.gz", | |
| ], | |
| ) |
There was a problem hiding this comment.
Code Review
This pull request correctly addresses a dependency shadowing issue by upgrading rules_python to version 0.25.0 and moving its declaration to a more prominent position in the WORKSPACE file. This improves the build's predictability and maintainability. My review includes a suggestion to further improve the dependency declaration by using urls with a mirror, which enhances resilience against network failures and aligns with best practices observed elsewhere in the project.
| http_archive( | ||
| name = "rules_python", | ||
| sha256 = "5868e73107a8e85d8f323806e60cad7283f34b32163ea6ff1020cf27abef6036", | ||
| strip_prefix = "rules_python-0.25.0", | ||
| url = "https://github.com/bazelbuild/rules_python/releases/download/0.25.0/rules_python-0.25.0.tar.gz", | ||
| ) |
There was a problem hiding this comment.
For better resilience against network issues and for consistency with other dependency declarations in this workspace (like rules_java), it's a good practice to use urls with a mirror. This provides a fallback if the primary URL is unavailable.
| http_archive( | |
| name = "rules_python", | |
| sha256 = "5868e73107a8e85d8f323806e60cad7283f34b32163ea6ff1020cf27abef6036", | |
| strip_prefix = "rules_python-0.25.0", | |
| url = "https://github.com/bazelbuild/rules_python/releases/download/0.25.0/rules_python-0.25.0.tar.gz", | |
| ) | |
| http_archive( | |
| name = "rules_python", | |
| sha256 = "5868e73107a8e85d8f323806e60cad7283f34b32163ea6ff1020cf27abef6036", | |
| strip_prefix = "rules_python-0.25.0", | |
| urls = [ | |
| "https://mirror.bazel.build/github.com/bazelbuild/rules_python/releases/download/0.25.0/rules_python-0.25.0.tar.gz", | |
| "https://github.com/bazelbuild/rules_python/releases/download/0.25.0/rules_python-0.25.0.tar.gz", | |
| ], | |
| ) |
|
needs non-root forge first.. |
actually forge is already using non-root user. the issue is that the bazel query is performed inside the container rather than outside the container. |
Signed-off-by: Lonnie Liu <[email protected]>
7c9803d to
dda6f8c
Compare
|
|
||
|
|
||
| def _get_new_tests(prefix: str, container: TesterContainer) -> Set[str]: | ||
| def _get_new_tests(prefix: str, workspace_dir: str) -> Set[str]: |
There was a problem hiding this comment.
Bug: Signature mismatch breaks test suite.
The function signature of _get_new_tests was changed from accepting a TesterContainer to accepting a workspace_dir string, but the corresponding test in test_tester.py at line 300-302 was not updated and still passes a LinuxTesterContainer object. This breaks the existing test suite and will cause test failures.
elliot-barn
left a comment
There was a problem hiding this comment.
Can you update the unit tests in test_tester.py
|
This pull request has been automatically marked as stale because it has not had You can always ask for help on our discussion forum or Ray's public slack channel. If you'd like to keep this open, just leave any comment, and the stale label will be removed. |
|
This pull request has been automatically marked as stale because it has not had You can always ask for help on our discussion forum or Ray's public slack channel. If you'd like to keep this open, just leave any comment, and the stale label will be removed. |
|
This pull request has been automatically marked as stale because it has not had You can always ask for help on our discussion forum or Ray's public slack channel. If you'd like to keep this open, just leave any comment, and the stale label will be removed. |
previously it was actually using 0.4.0, which is set up by the grpc repo. the declaration in the workspace file was being shadowed..