-
Notifications
You must be signed in to change notification settings - Fork 5
ci: Improve weekly download flow #381
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
/lgtm review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🦉 lgtm Review
Score: Nitpicks 🤓
🔍 Summary
This is a high-quality PR that significantly improves the robustness and testability of the weekly package download script.
The GitHub Actions workflow has been improved by separating the download steps and using continue-on-error for better error handling. The download script itself has been refactored for better testability and now includes a solid retry mechanism and validation for API responses.
The addition of a comprehensive test suite is a major highlight. However, I found an issue in the pagination test (test_npm_download_with_multiple_pages) where the assertions are incorrect due to how mutable dictionaries are handled by the mocking library. I've left comments with suggestions on how to fix both the application code to make it more easily testable and the test itself.
Overall, excellent work. Once the testing issue is addressed, this PR will be in great shape.
More information
- Id:
e58bb0b47d5641b790ee5aceacd46a60 - Model:
gemini-2.5-pro - Created at:
2025-11-03T14:08:17.016642+00:00
Usage summary
- Request count:
2 - Request tokens:
66,141 - Response tokens:
13,890 - Total tokens:
80,031
See the 📚 lgtm-ai repository for more information about lgtm.
| wait_exp_base=RETRY_WAIT_EXP_BASE, | ||
| wait_max=RETRY_WAIT_MAX, | ||
| ): | ||
| with attempt, httpx.Client(timeout=TIMEOUT) as client: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does not really matter, but we could instantiate the http client once and shave off an irrelevant amount of time.
This PR increases the number of retries to be performed during the download of packages, as well as modifies the CI job to use
continue-on-errorand adds some tests, since the functionality is growing.