Skip to content
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

Upgrade bot to be compatible with PyGithub v2.1.1 #224

Merged
merged 8 commits into from
Nov 9, 2023

Conversation

laraPPr
Copy link
Collaborator

@laraPPr laraPPr commented Oct 20, 2023

I tested the changes with python 3.8

resolves the following issue: #223

@laraPPr laraPPr changed the title Upgrade bot to be compatible with PyGithub Upgrade bot to be compatible with PyGithub v2.1.1 Oct 20, 2023
@boegel
Copy link
Contributor

boegel commented Oct 20, 2023

PyGithub 2.1.1 requires Python >= 3.7, so applying this fix implies that the bot will no longer support Python 3.6 (see also failing CI)...

That's painful because Python 3.6 is the default Python in RHEL 8 & derivates, so ideally we retain compatibility with that.

I wonder if there's a smart way in which can check whether or not we need to compare with a datetime instance that includes tzinfo, or not.
Maybe we can easily check which version of PyGithub we're dealing with?

@boegel
Copy link
Contributor

boegel commented Oct 20, 2023

Hmm, I can't find a direct way to check the PyGithub version, but we can do it by proxy: the GithubRetry class was introduced in PyGithub 2.x, so:

[bot@login1 ~]$ python3.6 -m pip list | grep PyGithub
PyGithub (1.56)
[bot@login1 ~]$ python3.6 -c "import github; print(hasattr(github, 'GithubRetry'))"
False
[boegel@login1 ~]$ python3.9 -m pip list | grep PyGithub
PyGithub           2.1.1
[boegel@login1 ~]$ python3.9 -c "import github; print(hasattr(github, 'GithubRetry'))"
True

Comment on lines 106 to 111
if hasattr(github, 'GithubRetry') is False:
if not _gh or (_token and datetime.utcnow() > _token.expires_at):
_gh = connect()
elif hasattr(github, 'GithubRetry') is True:
if not _gh or (_token and datetime.now(timezone.utc) > _token.expires_at):
_gh = connect()
Copy link
Contributor

@boegel boegel Oct 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if ... is False is kind of silly, and positive logic is easier to follow, and there's a lot of copy-pasting, so I would change this to:

    # PyGithub 2.x expects a datetime object with timezone info,
    # PyGithub 1.x requires one without timezone info;
    # see also https://github.com/PyGithub/PyGithub/pull/2565
    if hasattr(github, 'GithubRetry'):
        # PyGithub 2.x
        time_now = datetime.now(timezone.utc)
    else:
        # PyGithub 1.x
        time_now = datetime.utcnow()

    if not _gh or (_token and time_now > _token.expires_at):
        _gh = connect()

Copy link
Contributor

@boegel boegel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@trz42 trz42 merged commit a512f2e into EESSI:develop Nov 9, 2023
6 checks passed
@trz42
Copy link
Contributor

trz42 commented Nov 14, 2023

Did some test runs with Python/3.6.8 and Python/3.9.16. See trz42/software-layer#67

Looks good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants