-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
external: remove support of python2 for external clusters #14243
Conversation
2d48ddf
to
d789b40
Compare
I am importing packages,
And pylint is treating them as variables,
used here
Looks like a pylint bug, @rkachach @NymanRobin ?? |
@@ -34,7 +32,8 @@ | |||
if sys.version_info.major >= 3: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error originates from here. In this case, the "variables" are actually a function and a class, so the error message could indeed be improved. However, if called with Python 2, this will throw an error. Therefore, I believe the style error is still valid. The example below, tries to highlight the issue
if True == False:
foo = "bar"
print(foo)
The easiest way to fix this is to give the foo a baseline value and the style error and the potential bug is removed
foo = ""
if True == False:
foo = "bar"
print(foo)
However, in this particular example it is a bit harder as the ipaddress library was added in python 3.3
Therefore, my suggestion in the issue was to completely drop Python 2 support. This would resolve the style issue and make the code more readable by removing the if-statement that checks for the Python version. While defining the "variables" as None is an option, it wouldn't make the Python 2 code functional. It seems that the current code cannot fully operate with Python 2 anyway, as these two variables are not defined. It might be partially functional at best. 😄
The other option I would consider if for some reason we care about supporting python2 is rewriting this functions for example using the sockets library
Hope this is helpful!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want to keep the python2 support
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, interesting. I think Python 2 probably should be part of the tests somehow then also otherwise this is very easy to break.
I think the best plan of action would be to remove the dependencies on ipaddress and use socket instead. Something like socket.inet_pton(socket.AF_INET, ip_str) should be able to determine the IPv type by checking if it throws error. What do you think about this approach, @parth-gr? Do you think this would be a feasible fix?
I also found this as an option: https://github.com/phihag/ipaddress, but I think that would be quite hard to make explicit as there is no real dependency management, especially in the Python 2 case.
Others might of course have some other opinions on how this should be fixed, which are more than welcome. 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@parth-gr IMHO dropping Python2 support is something you may seriously consider as it's has been EOL for more than 4 years: https://devguide.python.org/versions/. All modern distros come with Python3 so I don't see the need to stick to a very old version with is already EOL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you remind me why we want to keep py2 support? Supporting only py3 would vastly simplify things.
IIRC, this had something to do with some Ceph versions/containers that still relied on py2. However, it's been a while since we talked about this, and things have a tendency to change over time. It's possible this isn't as necessary anymore, and this seems like the right time to revisit the discussion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it is mainly depend on the RHCS version, what it supports
Yes I agree @parth-gr the imports are not variables, but that is maybe a bit more cosmetic than functional but could be discussed with pylint community how easy it is to detect the "type" of import. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you update the commit so that
- the subject is more specific than 'pylint issue', and
- add text in the subject to clarify what the commit changes ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@NymanRobin and @rkachach are raising very good points in this thread around py2, and I would like to find some resolution here.
IMO, this is a great time to revisit py2 requirements. If, after review, we are still confident that we need to support py2, we should also determine if risk is great enough for us to consider adding a follow-up issue to add py2 to the test matrix.
b552d36
to
1a5d884
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome that the python2 could be removed!
959d599
to
7078540
Compare
I think this looks good, but can the commits be squashed? It doesn't make sense to me to make a bunch of changes in c1 and then undo the need for them entirely in an immediate follow-up commit. |
Done |
fix pylint issue and remove pyhn2 support closes: rook#14212 Signed-off-by: parth-gr <partharora1010@gmail.com>
Looks good, and I'm glad to have @NymanRobin 's help and thumbs-up here. Thanks :) |
Commit1: Fix the pylint CI
Commit2: Remove support of python2 for external clusters
closes: #14212
Checklist: