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

Rely on Python 2.5+ context managers #155

Merged
merged 2 commits into from
Jul 16, 2024
Merged

Conversation

ekohl
Copy link
Member

@ekohl ekohl commented Apr 25, 2024

In Python 2.5 context managers were added, which can help ensure open files are closed. EL 5 shipped with Python 2.4, but EL6 has Python 2.6 meaning we can now rely on this shiny new feature.

@ekohl
Copy link
Member Author

ekohl commented Apr 28, 2024

I think it's pulling in pulp 2 in the containers

except IOError:
error = sys.exc_info()[1] # backward and forward compatible way to get the exception
if error.errno == errno.ENOENT:
return False
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
except IOError:
error = sys.exc_info()[1] # backward and forward compatible way to get the exception
if error.errno == errno.ENOENT:
return False
except IOError as exc:
if exc.errno == errno.ENOENT:
return False

Should work on 2.7+

Copy link
Member

Choose a reason for hiding this comment

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

What I don't get is why this is special-cased at all -- if there was an IOError, the cache is not valid.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, if there was any error/exception, it's not valid. The only difference the code right now makes is returning False vs None, but nobody cares about that.

Copy link
Member Author

Choose a reason for hiding this comment

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

I wasn't quite sure about it and needing Python 2.6 at the time. Did we completely drop EL6 support, including in downstream?

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated to simplify. How about this version?

Copy link
Member

Choose a reason for hiding this comment

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

RHEL6 is dead-dead-dead soon: https://access.redhat.com/articles/4665701
And those changes won't land downstream before that anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

What's the soon part? I see since July 1st it's in a "we provide you with docs so you can upgrade" phase.

Copy link
Member

Choose a reason for hiding this comment

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

oh, just realized it's july already :D

@ekohl ekohl force-pushed the drop-old-python branch from 9310cc3 to 30fb515 Compare July 16, 2024 11:40
ekohl added 2 commits July 16, 2024 14:07
This removes a special case for ENOENT where it returned False,
otherwise None. In practice this doesn't matter and is handled in the
same way by callers.
In Python 2.5 context managers were added, which can help ensure open
files are closed. EL 5 shipped with Python 2.4, but EL6 has Python 2.6
meaning we can now rely on this shiny new feature.
@ekohl ekohl force-pushed the drop-old-python branch from 30fb515 to c343bbf Compare July 16, 2024 12:09
@ekohl ekohl merged commit a424d58 into Katello:master Jul 16, 2024
6 of 8 checks passed
@ekohl ekohl deleted the drop-old-python branch July 16, 2024 13:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants