search

fugue / credstash

A little utility for managing credentials in the cloud
Apache License 2.0
2.06k stars 214 forks source link

Python 2 support #269

Closed gruebel closed 4 years ago

gruebel commented 4 years ago

Hi,

wIth the changes from #263 the compatibility with Python 2 was broken. The problematic code part https://github.com/fugue/credstash/blob/2dec65f1070f5c17d46024f40ce5b448ea1b5196/credstash.py#L73 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 https://github.com/fugue/credstash/blob/2dec65f1070f5c17d46024f40ce5b448ea1b5196/credstash.py#L15 https://github.com/fugue/credstash/blob/2dec65f1070f5c17d46024f40ce5b448ea1b5196/credstash.py#L30-L33

corrjo commented 4 years ago

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 commented 4 years ago

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

tehamacrane commented 4 years ago

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 commented 4 years ago

Any update on when this will be fixed?

mike-luminal commented 4 years ago

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