drenv: Add automatic kustomize installation#2538
drenv: Add automatic kustomize installation#2538raaizik wants to merge 1 commit intoRamenDR:mainfrom
Conversation
raaizik
changed the title
nirs
left a comment
nirs
left a comment
There was a problem hiding this comment.
Since the code for specific tool is pretty simple and we can reuse most code, we can start with single drenv/tools.py.
The public API for the module is the setup() function. We don't need anything else public.
Not sure why we have tools_kustomize.py when all the other tools are classes in tools.py.
nirs
left a comment
nirs
left a comment
There was a problem hiding this comment.
The tools.py module duplicate the same code for every tool. We ned to implement once install from single binary and from tar file and keep the needed configuration (download_url, tar_member_name) in a class attribute.
nirs
left a comment
nirs
left a comment
There was a problem hiding this comment.
Added move comments fro the tests.
nirs
left a comment
nirs
left a comment
There was a problem hiding this comment.
Added move comments for the tests.
This commit addresses PR RamenDR#2538 review comments by refactoring the tool management system with a minimal kustomize-only implementation. This incremental approach allows for focused review before extending to other tools. Key changes addressing review comments: 1. Tool base class improvements: - name and required_version are now class attributes (not instance) - install_dir() is now a method returning venv bin directory - Removed instance variables in favor of class-level declarations 2. Error handling: - Use drenv.commands.run() instead of subprocess directly - Narrow error handling: only catch FileNotFoundError for missing tools - Let other errors propagate to avoid hiding bugs - Work around commands.Error.with_exception() setting __cause__ to None 3. Python idioms: - Use 'f' for file handles (not 'file') - Use tarfile mode 'r' for auto-detection instead of 'r:gz' - Simplified version parsing for kustomize output format 4. Code organization: - Removed binary name guessing - each tool knows its exact name - Imports organized at module top level - Single tools.setup() call in __main__.py 5. Implementation scope: - Start with kustomize only (other tools removed temporarily) - Updated __init__.py to export only Tool, Kustomize, setup - Removed tools_kustomize.py (functionality merged into tools.py) - Renamed test_tools_kustomize.py to test_tools.py for generality Testing: - Verified kustomize installation on macOS - Verified version detection works correctly - Verified upgrade detection (version comparison) - Integration with drenv setup command confirmed Related: RamenDR#1493 Addresses: PR RamenDR#2538 review comments from nirs
raaizik
force-pushed
the
issue-1493-drenv-tools
branch
8 times, most recently
from
f368eeb to
e04fbdf
Compare
raaizik
force-pushed
the
issue-1493-drenv-tools
branch
from
e04fbdf to
4ac56a7
Compare
raaizik
force-pushed
the
issue-1493-drenv-tools
branch
6 times, most recently
from
9f7e166 to
09211fc
Compare
Implements automatic installation of kustomize tool for drenv: - New tools module in test/drenv/tools/ - Kustomize class with version detection and installation - Integration with 'drenv setup' command This is the first tool in the series. Future commits will add support for the rest. Related: RamenDR#1493 Signed-off-by: raaizik <132667934+raaizik@users.noreply.github.com>
raaizik
force-pushed
the
issue-1493-drenv-tools
branch
from
f93a1e2 to
c96aaae
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughThis PR adds a tool-management system to the drenv test infrastructure. It introduces a ChangesTool Management System for drenv
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
test/drenv/tools/tools.py (1)
182-188:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
install()leaves a partial binary ininstall_dir()if extraction fails
extract_tarwrites the binary directly toself.install_dir(). A network error, disk-full condition, orFilterErrormid-extraction leaves a truncated/corrupted binary at the final path. Subsequentversion()calls will then either crash or mis-report the version.The fix is to extract into
tmpdir(already created for the archive download) and atomically rename to the final path on success:🛡️ Proposed fix
def install(self): """Install kustomize from GitHub releases.""" with tempfile.TemporaryDirectory() as tmpdir: archive_path = os.path.join(tmpdir, "kustomize.tar.gz") download(self.download_url, archive_path) - extract_tar(archive_path, self.install_dir(), self.archive_binary_path) - os.chmod(self.path(), 0o755) + extract_tar(archive_path, tmpdir, self.archive_binary_path) + tmp_binary = os.path.join(tmpdir, self.name) + os.chmod(tmp_binary, 0o755) + shutil.move(tmp_binary, self.path())🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/drenv/tools/tools.py` around lines 182 - 188, The install() method currently extracts the binary directly into self.install_dir(), risking a partial/corrupt binary on extraction failure; change install() to extract into the existing temporary directory (tmpdir) first (use extract_tar to a temp destination inside tmpdir using self.archive_binary_path as the extracted name), verify extraction succeeded, then atomically move/rename the temp binary into its final location returned by self.path()/self.install_dir() (e.g., os.replace or similar) and set permissions with os.chmod only after the atomic rename; ensure any temporary extracted file is cleaned up on error so a truncated file is never left at the final path.
🧹 Nitpick comments (1)
test/drenv/tools/tools_test.py (1)
128-137: ⚡ Quick win
test_setup_accepts_install_dirnever exercises theinstall_dirparameter
callable(tools.setup)is alwaysTruefor a defined function — this test provides no coverage whatsoever. The past reviewer's intent was specifically to useinstall_dir=to runsetup()without touching the real venv. That path is still untested.♻️ Proposed replacement
class TestSetup(unittest.TestCase): """Test setup function.""" def test_setup_accepts_install_dir(self): - """Test setup accepts custom install_dir parameter.""" - # This test verifies the parameter is accepted - # Actual installation testing would require more setup - # Just verify setup() accepts the parameter without error - # We don't actually call it to avoid modifying the system - self.assertTrue(callable(tools.setup)) + """Test setup installs tools into a custom install_dir.""" + with tempfile.TemporaryDirectory() as tmpdir: + tools.setup(install_dir=tmpdir) + kustomize_bin = os.path.join(tmpdir, "kustomize") + self.assertTrue(os.path.isfile(kustomize_bin)) + self.assertTrue(os.access(kustomize_bin, os.X_OK))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/drenv/tools/tools_test.py` around lines 128 - 137, Replace the no-op assertion with an actual call to tools.setup using an isolated temp dir: in test_setup_accepts_install_dir, create a temporary directory via tempfile.TemporaryDirectory() (or pytest tmp_path), call tools.setup(install_dir=temp_dir) and assert it completes (no exception) and/or that the returned value or side-effect references the temp_dir (e.g., os.path.exists(temp_dir) or returned_path.startswith(temp_dir)). If tools.setup performs external actions, wrap the call in mocks for those helpers or teardown the temp dir after the test to avoid touching the real environment; keep the test focused on exercising the install_dir parameter and cleaning up the temp directory.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/drenv/tools/tools.py`:
- Around line 138-143: The tar.extract call in the install routine is missing
the filter argument which triggers DeprecationWarning; update the extraction to
call tar.extract(member, target_dir, filter='data') so the archive extraction of
the single binary (the code that uses tar.getmember(archive_binary_path), sets
member.name = os.path.basename(member.name), and then calls tar.extract(...))
uses the safer 'data' filter for trusted GitHub release archives.
In `@test/test_tools.py`:
- Around line 13-16: The import order is wrong: the sys.path.insert(0, ".") call
currently runs after the import "from drenv.tools import tools", so it never
affects that import; move the sys.path.insert(0, ".") statement to before the
"from drenv.tools import tools" line (or remove the sys.path manipulation
entirely if drenv is guaranteed to be installed in the test venv) so that the
import can succeed when loading drenv from the repository.
---
Duplicate comments:
In `@test/drenv/tools/tools.py`:
- Around line 182-188: The install() method currently extracts the binary
directly into self.install_dir(), risking a partial/corrupt binary on extraction
failure; change install() to extract into the existing temporary directory
(tmpdir) first (use extract_tar to a temp destination inside tmpdir using
self.archive_binary_path as the extracted name), verify extraction succeeded,
then atomically move/rename the temp binary into its final location returned by
self.path()/self.install_dir() (e.g., os.replace or similar) and set permissions
with os.chmod only after the atomic rename; ensure any temporary extracted file
is cleaned up on error so a truncated file is never left at the final path.
---
Nitpick comments:
In `@test/drenv/tools/tools_test.py`:
- Around line 128-137: Replace the no-op assertion with an actual call to
tools.setup using an isolated temp dir: in test_setup_accepts_install_dir,
create a temporary directory via tempfile.TemporaryDirectory() (or pytest
tmp_path), call tools.setup(install_dir=temp_dir) and assert it completes (no
exception) and/or that the returned value or side-effect references the temp_dir
(e.g., os.path.exists(temp_dir) or returned_path.startswith(temp_dir)). If
tools.setup performs external actions, wrap the call in mocks for those helpers
or teardown the temp dir after the test to avoid touching the real environment;
keep the test focused on exercising the install_dir parameter and cleaning up
the temp directory.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9786e58e-2219-408b-90fa-21a7864956d6
📒 Files selected for processing (5)
test/drenv/__main__.pytest/drenv/tools/__init__.pytest/drenv/tools/tools.pytest/drenv/tools/tools_test.pytest/test_tools.py
📜 Review details
🧰 Additional context used
🪛 Ruff (0.15.12)
test/test_tools.py
[warning] 28-28: Do not catch blind exception: Exception
(BLE001)
test/drenv/tools/__init__.py
[warning] 20-24: __all__ is not sorted
Apply an isort-style sorting to __all__
(RUF022)
test/drenv/tools/tools_test.py
[error] 44-44: Probable insecure usage of temporary file or directory: "/tmp/custom"
(S108)
test/drenv/tools/tools.py
[error] 123-123: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
[error] 188-188: os.chmod setting a permissive mask 0o755 on file or directory
(S103)
🔇 Additional comments (3)
test/drenv/tools/__init__.py (1)
1-24: LGTMClean re-export package; the module docstring,
__all__, and imports are all consistent with the implementation intools.py.test/drenv/__main__.py (1)
28-28: LGTMBoth past review concerns are fully addressed:
toolsis imported at module level (Line 28), anddo_setupdelegates to the singletools.setup()call rather than importing tools inline or listing them individually.Also applies to: 344-355
test/drenv/tools/tools.py (1)
164-180: ⚡ Quick winRemove this comment — the exception handling is correct as-is
commands.run()wraps allOSErrorexceptions (includingFileNotFoundError) in acommands.Errorat line 136 oftest/drenv/commands.py:except OSError as e: raise Error(args, f"Could not execute: {e}").with_exception(e)When a binary doesn't exist,
subprocess.PopenraisesFileNotFoundError(a subclass ofOSError), which is caught and wrapped.FileNotFoundErrornever propagates directly fromcommands.run(). The current code correctly catchescommands.Errorand checks the message. CatchingFileNotFoundErrordirectly inversion()would never match because the exception is always wrapped.The "install missing tool" path is not dead — the exception handling works as intended.
> Likely an incorrect or invalid review comment.
| with tarfile.open(archive_path, "r") as tar: | ||
| member = tar.getmember(archive_binary_path) | ||
| # Extract binary to target_dir root, removing any directory structure from archive | ||
| # This ensures the binary is placed directly in install_dir, not in a subdirectory | ||
| member.name = os.path.basename(member.name) | ||
| tar.extract(member, target_dir) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Python 3.12 3.13 tar.extract DeprecationWarning filter parameter
💡 Result:
In Python 3.12 and 3.13, calling tarfile.TarFile.extract or extractall without the filter parameter (or with filter=None) triggers a DeprecationWarning: "Python 3.14 will, by default, filter extracted tar archives and reject files or modify their metadata. Use the filter argument to control this behavior." [1][2][3] This behavior was introduced in Python 3.12 via PEP 706 to address security issues like CVE-2007-4559 (path traversal in tar extraction). When no filter is specified, it falls back to the 'fully_trusted' filter (matching pre-3.12 behavior) but emits the warning. The default changes in Python 3.14 to 'data', which blocks dangerous features like absolute paths, symlinks outside the target directory, and certain metadata. [1][4][2][3] To silence the warning and prepare for 3.14: - Use filter='data' for safe extraction of data archives (recommended for most cases, ignores/block most UNIX-specific features). [1][3] - Use filter='fully_trusted' if you need full legacy behavior and trust the archive. [2] - Use filter='tar' to honor tar-specific features but block highly dangerous ones. [1] - Or set TarFile.extraction_filter globally/class-wide, e.g., TarFile.extraction_filter = 'data' [1][3] Example: import tarfile with tarfile.open('archive.tar') as tf: tf.extractall(path='dest', filter='data') # No warning, safe for 3.14+ Python 3.13 retains this exact deprecation behavior (no changes noted in 3.13 docs or changelog); the switch happens in 3.14. [1][5] Note: There was a CVE-2024-12718 in 3.12+ filters='data'/'tar' allowing limited metadata changes outside dest, but patches are available—upgrade Python. [6][7]
Citations:
- 1: https://docs.python.org/3.13/library/tarfile.html
- 2: https://docs.python.org/3.12/library/tarfile.html
- 3: https://peps.python.org/pep-0706/
- 4: https://docs.python.org/3/library/tarfile.html
- 5: https://docs.python.org/3/whatsnew/3.13.html
- 6: https://www.upwind.io/feed/cve-2024-12718-path-escape-via-pythons-tarfile-extraction-filters
- 7: https://www.sweet.security/blog/python-tar-file-vulnerability-cve-2024-12718-what-you-need-to-know
🌐 Web query:
Python 3.14 tarfile extract filter default behavior changes
💡 Result:
In Python 3.14, the default behavior of tarfile.TarFile.extract and extractall changed significantly regarding the filter parameter. Previously (in 3.12 and 3.13), when the filter argument was None (the default), it would use the 'fully_trusted' filter after emitting a DeprecationWarning, allowing potentially dangerous operations like extracting to absolute paths or paths outside the destination directory [1][2]. In Python 3.14, the default filter is now 'data', which disallows such dangerous features including links to absolute paths or paths outside the destination, making extraction safer by default [1][3]. This implements PEP 706 [2][4]. The official documentation states: "Changed in version 3.14: The default filter is set to data, which disallows some dangerous features such as links to absolute paths or paths outside of the destination. Previously, the default was equivalent to fully_trusted." [1]. Developers are recommended to explicitly set the filter if specific behavior is needed, especially for compatibility across versions [1]. Note that even the 'data' filter does not prevent all security issues, as seen in later CVEs like CVE-2024-12718, but it addresses the most critical path traversal risks [5][6][3].
Citations:
- 1: https://docs.python.org/3.14/library/tarfile.html
- 2: Fully implement PEP 706: Change default filter for TarFile.extract() and extractall() to 'data' python/cpython#121999
- 3: https://docs.python.org/3/whatsnew/3.14.html
- 4: Update tarfile extraction for Python 3.14 compatibility conda/conda-build#5912
- 5: https://www.upwind.io/feed/cve-2024-12718-path-escape-via-pythons-tarfile-extraction-filters
- 6: https://www.sweet.security/blog/python-tar-file-vulnerability-cve-2024-12718-what-you-need-to-know
🏁 Script executed:
# Check the context around the code to understand if filter='data' is appropriate
cat -n test/drenv/tools/tools.py | head -160 | tail -40Repository: RamenDR/ramen
Length of output: 1741
Add filter='data' to tar.extract() to silence DeprecationWarning and prepare for Python 3.14
Without an explicit filter argument, tar.extract() emits a DeprecationWarning on Python 3.12–3.13 and will use a safer default ('data') in Python 3.14. Since the archive is from a trusted GitHub release and contains only a single binary, filter='data' is the recommended approach.
Fix
- tar.extract(member, target_dir)
+ tar.extract(member, target_dir, filter='data')🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/drenv/tools/tools.py` around lines 138 - 143, The tar.extract call in
the install routine is missing the filter argument which triggers
DeprecationWarning; update the extraction to call tar.extract(member,
target_dir, filter='data') so the archive extraction of the single binary (the
code that uses tar.getmember(archive_binary_path), sets member.name =
os.path.basename(member.name), and then calls tar.extract(...)) uses the safer
'data' filter for trusted GitHub release archives.
| from drenv.tools import tools | ||
|
|
||
| # Add test directory to path so we can import drenv | ||
| sys.path.insert(0, ".") |
There was a problem hiding this comment.
sys.path.insert is dead code — it executes after the import it was meant to enable
The from drenv.tools import tools import on Line 13 runs before sys.path.insert(0, ".") on Line 16. If drenv is not already importable, the script errors at Line 13, never reaching Line 16. The path manipulation should either be moved above the imports or removed entirely if drenv is guaranteed to be installed in the active venv.
🐛 Proposed fix
+# Add test directory to path so we can import drenv
+sys.path.insert(0, ".")
+
from drenv.tools import tools
-
-# Add test directory to path so we can import drenv
-sys.path.insert(0, ".")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| from drenv.tools import tools | |
| # Add test directory to path so we can import drenv | |
| sys.path.insert(0, ".") | |
| # Add test directory to path so we can import drenv | |
| sys.path.insert(0, ".") | |
| from drenv.tools import tools |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/test_tools.py` around lines 13 - 16, The import order is wrong: the
sys.path.insert(0, ".") call currently runs after the import "from drenv.tools
import tools", so it never affects that import; move the sys.path.insert(0, ".")
statement to before the "from drenv.tools import tools" line (or remove the
sys.path manipulation entirely if drenv is guaranteed to be installed in the
test venv) so that the import can succeed when loading drenv from the
repository.
2 participants
Change
Implements automatic installation of kustomize tool for drenv:
This is the first tool in the series. Future commits will add support for the rest.
Related: #1493
Test results
No kustomize installed
Setup
Test
Verification
Old kustomize version (5.3.0)
Setup
Test
Verification
Correct version already in venv (5.7.0)
Setup
Test
Verification
System kustomize with newer version (5.8.1)
Setup
Test
Verification
Integration test: drenv setup
Setup
Test
TODOs
Summary by CodeRabbit
New Features
Tests