Cicd fix #3088
Cicd fix #3088
Conversation
|
There was a problem hiding this comment.
Pull request overview
This PR refactors the Python test infrastructure to fix CI/CD issues related to module imports and PostgreSQL permissions when running as root. The changes reorganize test utilities into a proper test_utils package and add support for running tests in containerized CI environments.
Key changes:
- Reorganized test utilities from
libtotest_utilspackage with proper__init__.py - Added permission handling for PostgreSQL operations when running as root user in CI/CD
- Updated test fixtures to use proper temporary directories with correct ownership
- Removed disabled PubMed test file
Reviewed changes
Copilot reviewed 39 out of 39 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| postgres/tests/py_tests/test_utils/init.py | Creates new test_utils package with proper exports |
| postgres/tests/py_tests/test_utils/assertions.py | Moves Assertions class to new package location |
| postgres/tests/py_tests/test_utils/helpers.py | Moves helper functions to new package location |
| postgres/tests/py_tests/test_*.py (multiple files) | Updates imports from lib.assertions to test_utils.assertions |
| postgres/tests/py_tests/conftest.py | Adds root user permission handling and temp directory fixture |
| postgres/tests/py_tests/test_root_path.py | Updates to use new temp_dir_for_postgres fixture |
| postgres/tests/py_tests/test_pubmed_table.py | Removes disabled test file |
| postgres/tests/py_tests/requirements.txt | Adds numpy dependency |
| postgres/tests/Makefile | Removes PYTHONPATH=. from pytest commands |
| .github/workflows/pg-extension-build.yaml | Adds postgres directory ownership and removes PYTHONPATH |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| from typing import Dict, AsyncGenerator | ||
|
|
||
| # Ensure the test directory is in Python path for lib imports | ||
| sys.path.insert(0, str(Path(__file__).parent)) |
There was a problem hiding this comment.
Adding the test directory to sys.path can cause import conflicts and is an anti-pattern. Since test_utils is now a proper package with init.py, Python's standard import resolution should handle it without path manipulation. Consider removing this line.
| sys.path.insert(0, str(Path(__file__).parent)) |
| shutil.chown(str(data_dir), user=user, group=user) | ||
| # Run initdb as postgres user using su | ||
| result = subprocess.run( | ||
| ["su", "-", user, "-c", f"{initdb} -D {data_dir} -U {user}"], |
There was a problem hiding this comment.
The command is constructed using f-string interpolation without proper shell escaping. If data_dir or user contain special shell characters, this could lead to command injection. Use a list of arguments instead of a shell command string, or properly escape the variables.
| # Run pg_ctl as postgres user with only essential environment variables | ||
| ld_library_path = env.get("LD_LIBRARY_PATH", "") | ||
| subprocess.run( | ||
| ["su", "-", user, "-c", f"LD_LIBRARY_PATH={ld_library_path} {pg_ctl} -D {data_dir} -l {log_file} start"], |
There was a problem hiding this comment.
The command string is vulnerable to shell injection if any of the path variables contain special characters. Use proper argument lists or shell escaping to prevent potential security issues.
| user = os.environ.get("USER", "postgres") | ||
| if os.geteuid() == 0: # Running as root | ||
| result = subprocess.run( | ||
| ["su", "-", user, "-c", f"{pg_ctl} status -D {data_dir}"], |
There was a problem hiding this comment.
The command construction is vulnerable to shell injection. Use proper argument lists or shell escaping for the path variables.
| user = os.environ.get("USER", "postgres") | ||
| if os.geteuid() == 0: # Running as root | ||
| subprocess.run( | ||
| ["su", "-", user, "-c", f"{pg_ctl} stop -D {data_dir} -m fast"], |
There was a problem hiding this comment.
The command string is vulnerable to shell injection if path variables contain special characters. Use proper argument lists or shell escaping.
|
|
||
| # Ensure subdirectories are owned by postgres user | ||
| if os.geteuid() == 0: | ||
| import shutil |
There was a problem hiding this comment.
The shutil module is imported inside a conditional block at line 104, but it's already used elsewhere in the codebase (e.g., in conftest.py). Consider moving this import to the top of the file with other imports for consistency and clarity.
🚀 🚀 Pull Request
Impact
Description
Things to be aware of
Things to worry about
Additional Context