Copilot commented on code in PR #8770:
URL: https://github.com/apache/hbase/pull/8770#discussion_r4275734921
##########
dev-support/read-replica/python/test/conftest.py:
##########
@@ -55,8 +62,10 @@ def per_test_log_file(request):
test_name = request.node.name
execution_count = getattr(request.node, 'execution_count', 1)
- run_dir = os.path.join(output_dir, test_name, f"run{execution_count}")
- os.makedirs(run_dir, exist_ok=True)
+ test_dir = os.path.join(output_dir, test_name)
+ run_dir = os.path.join(test_dir, f"run{execution_count}")
+ os.makedirs(run_dir, mode=0o777, exist_ok=True)
+ _chmod_recursive(test_dir)
Review Comment:
`_chmod_recursive(test_dir)` will traverse and chmod every prior run
directory for the test on each test invocation, which can become increasingly
expensive as artifacts accumulate (especially if logs are large). A more
efficient approach is to chmod only what was newly created/modified (e.g.,
`run_dir` and its immediate parents as needed), rather than the entire
`test_dir` tree on every test.
##########
dev-support/read-replica/python/test/conftest.py:
##########
@@ -38,6 +38,13 @@ def pytest_runtest_makereport(item, call):
setattr(item, f"rep_{report.when}", report)
+def _chmod_recursive(path, mode=0o777):
+ for dirpath, dirnames, filenames in os.walk(path):
+ os.chmod(dirpath, mode)
+ for filename in filenames:
+ os.chmod(os.path.join(dirpath, filename), mode)
Review Comment:
Using `0o777` recursively makes all files world-writable and also sets the
executable bit on regular log files, which is broader access than needed and
can be risky on shared CI hosts. To preserve the goal (allowing Jenkins to
delete artifacts), consider applying permissive permissions only to directories
(e.g., `0o777` or `0o775`) and using a non-executable mode for files (e.g.,
`0o666`/`0o664`) or using an `a+rwX`-style approach so execute is only added
where appropriate.
##########
dev-support/read-replica/run_read_replica_integration_tests.sh:
##########
@@ -259,6 +259,18 @@ cleanup() {
fi
echo "=== Cleanup: Deleting copied HBase directory: ${REPLICA_DIR}/hbase ==="
rm -rf "${REPLICA_DIR}/hbase"
+ echo "=== Cleanup: Deleting artifacts that would be owned by root on the CI
host ==="
+ rm -rf "${REPLICA_DIR}/.venv"
+ rm -rf "${REPLICA_DIR}/.pytest_cache"
+ rm -rf "${REPLICA_DIR}/python/test/.pytest_cache"
+ rm -rf "${REPLICA_DIR}/python/__pycache__"
+ rm -rf "${REPLICA_DIR}/python/test/__pycache__"
+ rm -rf "${REPLICA_DIR}/python/src/__pycache__"
+ rm -rf "${REPLICA_DIR}/python/proto/generated"
+ rm -rf "${REPLICA_DIR}/python/proto/ActiveClusterSuffix.proto"
+ rm -rf "${REPLICA_DIR}/tmp-read-replica-data"
+ rm -rf "${REPLICA_DIR}/cluster1/logs"
+ rm -rf "${REPLICA_DIR}/cluster2/logs"
Review Comment:
This cleanup list is getting long and repetitive, which makes it easier to
miss paths or introduce inconsistencies over time. Consider consolidating these
into a single list/array of relative paths and iterating over it (or using a
small helper) so additions/removals are less error-prone and easier to review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
