Bug-2038705: Migrate get_releases function to PyGithub and update tests - #9695
Bug-2038705: Migrate get_releases function to PyGithub and update tests#9695moijes12 wants to merge 8 commits into
Conversation
- Migrate the `get_releases` function of the `treeherder/utils/github.py` to PyGithub. - Updates tests in tests/changelog/ to update mocking of get_releases - Add tests for get_releases in a new tests/utils/test_github.py file Fixes Bug-2038705
0ad02f6 to
edacce4
Compare
|
Exclusively used by perfherder. |
The github.get_releases now filters first by published_at and then by the number of items expected. The relevant tests have been updated too.
|
@Archaeopteryx @gmierz I have updated the filtering logic to first filter by published_at and then by number of items. The relevant tests have been updated too. I had checked locally that the call to PyGithub's get_releases() returns the GitRelease objects with the oldest releases first. So it is better to first get only those releases that were published after the cutoff date and then get a slice of them. Doing the reverse would be incorrect. |
Use a timezone aware datetime object when setting the "since" parameter of changelog.
Add the timezone awareness to the changelog since parameter
Use tz aware timestamps in Changelog
Update
|
Improve the performance of get_releases by lazy loading of GitRelease objects.
|
Improved the performance by lazy loading of GitRelease objects. See result below |
`github.py` now converts the PyGithub objects into native python objects so that downstream modules do not need to handle them.
- Use timezone aware date strings in test comparisons. - Fix unit tests
|
Added the below updates too:
|
gmierz
left a comment
There was a problem hiding this comment.
Changes look good to me, thanks for testing locally too :)
Just one minor change below and it'll be good to land.
|
|
||
|
|
||
| def pygithub_get_repo(owner, repo): | ||
| def pygithub_get_repo(owner, repo) -> Repository: |
There was a problem hiding this comment.
I don't mind having these type hints, but given that we don't have them anywhere else in the file this makes it look odd. Can you remove them? We could add them in a separate PR if you want.
There was a problem hiding this comment.
I'd recommend keeping it. I wanted to start introducing them slowly rather than adding it in one big PR which would make it difficult to review. Let me know what you think
Changes Introduced
Testing
time python manage.py updated_changelog --days 12. It takes 23 seconds again 18 seconds taken by the old implementation.Fixes Bug-2038705
@gmierz @Archaeopteryx