Conversation
…grade-logic Fix safe minor upgrade when all minors vulnerable
TongWu
changed the title
|
Caution Review failedThe pull request is closed. WalkthroughThe changes introduce a new module for repacking Python wheel files to update their metadata, refactor the logic for suggesting safe package upgrades, and enhance configurability in report generation by using environment variables and unified timestamp handling. No changes were made to exported or public entity declarations outside of internal logic. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant WhlRepacker
participant FileSystem
User->>CLI: Run repack command with input/output dirs
CLI->>WhlRepacker: process_all_wheels(input_dir, output_dir)
WhlRepacker->>FileSystem: Find all .whl files in input_dir
loop For each wheel
WhlRepacker->>FileSystem: Extract wheel to temp dir
WhlRepacker->>FileSystem: Locate METADATA file
alt METADATA found
WhlRepacker->>FileSystem: Update Metadata-Version to 2.3
WhlRepacker->>FileSystem: Repack wheel to output_dir
else METADATA missing
WhlRepacker->>CLI: Log error
end
WhlRepacker->>FileSystem: Cleanup temp dir
end
WhlRepacker->>CLI: Done
Possibly related PRs
Suggested labels
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
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: 0
🧹 Nitpick comments (5)
utils/VersionSuggester.py (1)
77-79: Fix formatting issue flagged by static analysis.The function definition needs proper spacing according to PEP 8 standards.
+ async def suggest_safe_minor_upgrade( pkg: str, current_version: str, all_versions: list ) -> str:utils/WhlRepacker.py (4)
6-6: Fix formatting issues flagged by static analysis.The function definitions need proper spacing according to PEP 8 standards.
+ def repack_single_wheel(wheel_path: Path, output_dir: Path):
6-53: Consider refactoring to reduce complexity.The function has 16 local variables which exceeds the recommended limit. Consider extracting helper functions for metadata processing and wheel creation.
Here's a suggested refactor:
def repack_single_wheel(wheel_path: Path, output_dir: Path): """Repack a single wheel to use Metadata-Version: 2.3""" temp_dir = output_dir / (wheel_path.stem + '_unpacked') + try: + _extract_wheel(wheel_path, temp_dir) + metadata_file = _find_metadata_file(temp_dir) + if not metadata_file: + print(f"[ERROR] METADATA not found in: {wheel_path.name}") + return + + _update_metadata_version(metadata_file) + _create_repacked_wheel(temp_dir, wheel_path, output_dir) + print(f"[DONE] Repacked: {wheel_path.name}") + finally: + if temp_dir.exists(): + shutil.rmtree(temp_dir) + +def _extract_wheel(wheel_path: Path, temp_dir: Path): + with zipfile.ZipFile(wheel_path, 'r') as zip_ref: + zip_ref.extractall(temp_dir) + +def _find_metadata_file(temp_dir: Path) -> Path: + for root, _, files in os.walk(temp_dir): + for file in files: + if file == 'METADATA': + return Path(root) / file + return None + +def _update_metadata_version(metadata_file: Path): + with open(metadata_file, 'r', encoding='utf-8') as f: + lines = f.readlines() + + with open(metadata_file, 'w', encoding='utf-8') as f: + for line in lines: + if line.startswith("Metadata-Version:"): + f.write("Metadata-Version: 2.3\n") + else: + f.write(line) + +def _create_repacked_wheel(temp_dir: Path, original_wheel: Path, output_dir: Path): + new_wheel_path = output_dir / f"{original_wheel.stem}.whl" + with zipfile.ZipFile(new_wheel_path, 'w', zipfile.ZIP_DEFLATED) as zipf: + for root, _, files in os.walk(temp_dir): + for file in files: + file_path = Path(root) / file + arcname = file_path.relative_to(temp_dir) + zipf.write(file_path, arcname)
54-54: Fix formatting issue flagged by static analysis.Add proper spacing before function definition.
+ def process_all_wheels(input_dir: str, output_dir: str = None):
72-80: Fix formatting issue and improve script name in usage message.The script reference in the usage message doesn't match the actual filename.
+ if __name__ == "__main__": import sys if len(sys.argv) < 2: - print("Usage: python repack_wheels_batch.py <input_dir> [<output_dir>]") + print("Usage: python WhlRepacker.py <input_dir> [<output_dir>]") else: input_dir = sys.argv[1] output_dir = sys.argv[2] if len(sys.argv) > 2 else None process_all_wheels(input_dir, output_dir) +
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
MonthlyReport/2025-06/MonthlyReport-202506-26-0227.xlsxis excluded by!**/*.xlsxWeeklyReport/2025-06-23/WeeklyReport_20250626_101759.csvis excluded by!**/*.csv
📒 Files selected for processing (3)
GenerateReport.py(2 hunks)utils/VersionSuggester.py(1 hunks)utils/WhlRepacker.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
utils/VersionSuggester.py (1)
Learnt from: TongWu
PR: TongWu/PythonPackageManager#14
File: utils/VersionSuggester.py:136-137
Timestamp: 2025-06-24T03:17:27.141Z
Learning: In utils/VersionSuggester.py, the suggest_upgrade_version function has been intentionally disabled as it's outdated. The preferred approach is to use suggest_safe_minor_upgrade function instead for version suggestions.
🧬 Code Graph Analysis (2)
utils/VersionSuggester.py (1)
utils/VulnChecker.py (1)
fetch_osv(30-102)
GenerateReport.py (1)
utils/SGTUtils.py (1)
now_sg(25-27)
🪛 Flake8 (7.2.0)
utils/WhlRepacker.py
[error] 6-6: expected 2 blank lines, found 1
(E302)
[error] 54-54: expected 2 blank lines, found 1
(E302)
[error] 72-72: expected 2 blank lines after class or function definition, found 1
(E305)
utils/VersionSuggester.py
[error] 77-77: expected 2 blank lines, found 1
(E302)
🪛 Pylint (3.3.7)
utils/WhlRepacker.py
[refactor] 6-6: Too many local variables (16/15)
(R0914)
🔇 Additional comments (7)
utils/VersionSuggester.py (4)
80-85: LGTM! Improved documentation.The expanded docstring clearly explains the function's behavior, including the new logic for checking the next major release when all minor versions are vulnerable. This aligns well with the PR objective.
90-106: LGTM! Cleaner logic structure.The refactored approach of separating candidate versions into
minor_candidatesandhigher_majorlists makes the code more readable and maintainable compared to the previous implementation.
117-127: LGTM! Excellent implementation of PR objective.This new logic successfully implements the optimization described in the PR objectives: "if all minor versions of a package are found to be vulnerable, the system will now check the major version instead." The implementation correctly finds the next major version and checks for non-vulnerable versions there.
131-133: LGTM! Appropriate exception handling adjustment.Changing from
logger.errortologger.warningis appropriate since this function gracefully handles failures by returning "unknown" rather than causing the program to fail. The pragma comment correctly indicates this is hard to test in unit tests.utils/WhlRepacker.py (1)
24-27: Improve error handling and cleanup.The current implementation doesn't clean up the temp directory when METADATA is not found. Consider using a try-finally block or context manager.
- if not metadata_file or not metadata_file.exists(): - print(f"[ERROR] METADATA not found in: {wheel_path.name}") - shutil.rmtree(temp_dir) - return + if not metadata_file or not metadata_file.exists(): + print(f"[ERROR] METADATA not found in: {wheel_path.name}") + if temp_dir.exists(): + shutil.rmtree(temp_dir) + returnLikely an incorrect or invalid review comment.
GenerateReport.py (2)
119-130: LGTM! Excellent improvement in configurability.The change from hardcoded file path to using the
NOTUSED_PACKAGESenvironment variable significantly improves configurability. The enhanced logging that shows the actual file path being used is also helpful for debugging. This change addresses the PR objective for fixing the NotUsed.txt handling.
339-344: LGTM! Consistent timezone handling implementation.Using
now_sg()instead ofdatetime.now()ensures that the monthly report filenames use Singapore time (UTC+8), which aligns with the PR objective to make monthly report filenames consistent with weekly report filenames. Storing the timestamp in a separate variable also improves code readability.
TongWu
temporarily deployed
to
WT_WeeklyTriggerEnv
GitHub Actions
Inactive
TongWu
had a problem deploying
to
WT_WeeklyTriggerEnv
GitHub Actions
Error
1 participant
Summary by CodeRabbit
New Features
Improvements
Refactor