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

Python 2 support #269

Closed
gruebel opened this issue Feb 10, 2020 · 5 comments · Fixed by #282
Closed

Python 2 support #269

gruebel opened this issue Feb 10, 2020 · 5 comments · Fixed by #282
Milestone

Comments

@gruebel
Copy link

gruebel commented Feb 10, 2020

Hi,

wIth the changes from #263 the compatibility with Python 2 was broken. The problematic code part

if logger.hasHandlers():

could be easily fixed by removing that line completely, because there is no benefit in checking, if the logger has any handlers. If it doesn't have any the next line line would iterate over an empty list and therefore not remove any handler. I could create a simple PR, but I'm not sure, which Python versions credstash is officially supporting. If you don't support Python 2, then you could also remove all code related to it, like
from __future__ import print_function

credstash/credstash.py

Lines 30 to 33 in 2dec65f

try:
from StringIO import StringIO
except ImportError:
from io import StringIO

@corrjo
Copy link
Contributor

corrjo commented Feb 10, 2020

IMO python2 support should be removed. Not wise to try and support something that is already eol, and removing support will encourage those still using python 2 to move to 3.

@kenzliang
Copy link

@corrjo While true - it probably should be noted that this was a patch version release not a minor or major one.

@tehamacrane
Copy link

Credstash 1.16.2 is definitely broken. We've had to roll back to 1.16.1 on all of our images. Any word on when a fix will be in?

@eisjcormier
Copy link
Contributor

Any update on when this will be fixed?

@mike-luminal
Copy link
Contributor

Agreed that breaking Python 2 compatibility is inappropriate for a patch release. Will fix for next release.

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 a pull request may close this issue.

6 participants