-
Notifications
You must be signed in to change notification settings - Fork 132
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
Move to Python 3 #1316
Move to Python 3 #1316
Conversation
comp_ipv4_address = Combine(ipv4_oct + ('.' + ipv4_oct*3)) | ||
ipv4_address = Combine(ipv4_oct + ('.' + ipv4_oct*3)).setResultsName('ipv4_address') | ||
comp_ipv4_address = Combine(ipv4_oct + ('.' + ipv4_oct * 3)) | ||
ipv4_address = Combine(ipv4_oct + ('.' + ipv4_oct * 3)).setResultsName('ipv4_address') |
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.
line too long (94 > 79 characters)
local variable 'ipv4_address' is assigned to but never used
|
||
from errors import * | ||
from .errors 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.
'.errors.*' imported but unused
'from .errors import *' used; unable to detect undefined names
import logging | ||
import re | ||
|
||
import IPy | ||
|
||
from pyparsing import Combine, Forward, Group, Literal, nestedExpr, OneOrMore, ParseResults, quotedString, Regex, QuotedString, Word, ZeroOrMore, alphanums, nums, oneOf | ||
from pyparsing import Combine, Forward, Group, Literal, nestedExpr, OneOrMore, ParseResults, quotedString, Regex, \ |
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.
'pyparsing.OneOrMore' imported but unused
'pyparsing.alphanums' imported but unused
Black would make changes.
line too long (115 > 79 characters)
|
||
server_url = "http://unittest:[email protected]:1337/XMLRPC" | ||
s = xmlrpclib.Server(server_url, allow_none=1); | ||
s = xmlrpc.client.Server(server_url, allow_none=1); |
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.
statement ends with a semicolon
@@ -17,10 +17,10 @@ | |||
logger.setLevel(logging.DEBUG) | |||
log_format = "%(levelname)-8s %(message)s" | |||
|
|||
import xmlrpclib | |||
import xmlrpc.client |
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.
module level import not at top of file
|
||
def setupReqs(self): | ||
for i in range(0,self.concurrent_reqs): # make the initial pool of requests | ||
for i in range(0, self.concurrent_reqs): # make the initial pool of requests |
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.
Loop control variable 'i' not used within the loop body. If this is intended, start the name with an underscore.
line too long (85 > 79 characters)
raise Fault(1000, ("Function argument must be XML-RPC struct/Python dict (Python %s given)." % | ||
type(nipap_args).__name__ )) | ||
raise Fault(1000, ("Function argument must be XML-RPC struct/Python dict (Python {} given).".format( | ||
type(nipap_args).__name__ ))) |
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.
whitespace before ')'
|
||
from nipapconfig import NipapConfig | ||
from backend import Nipap, NipapError | ||
from .nipapconfig import NipapConfig |
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.
'.nipapconfig.NipapConfig' imported but unused
from flask import request, Response | ||
from flaskext.xmlrpc import XMLRPCHandler, Fault | ||
from flask_xmlrpcre.xmlrpcre import XMLRPCHandler, Fault | ||
from flask_compress import Compress |
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.
'flask_compress.Compress' imported but unused
from functools import wraps | ||
from flask import Flask | ||
from flask import Flask, current_app |
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.
'flask.Flask' imported but unused
@@ -1402,7 +1402,7 @@ def test_edit_pool(self): | |||
expected['tags'] = [] | |||
expected['avps'] = {} | |||
|
|||
self.assertEquals(self._mangle_pool_result(s.list_pool({ 'auth': ad, | |||
self.assertEqual(self._mangle_pool_result(s.list_pool({ 'auth': ad, |
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.
whitespace after '{'
As the authentication library is shared between nipapd and nipap-www, Python 3 support needs to be fixed for nipap-www as well before this can be merged. Unfortunately Pylons, the web framework used, lacks Python 3 support and is abandoned. To prepare for this, a migration from Pylons to Flask has been initiated. |
@@ -182,4 +182,4 @@ def drop_privileges(uid_name='nobody', gid_name='nogroup'): | |||
os.setuid(uid) | |||
|
|||
# Ensure a very conservative umask | |||
old_umask = os.umask(077) | |||
old_umask = os.umask(0o77) |
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.
local variable 'old_umask' is assigned to but never used
except OSError, e: | ||
raise Exception, "%s [%d]" % (e.strerror, e.errno) | ||
except OSError as e: | ||
raise Exception("%s [%d]" % (e.strerror, e.errno)) |
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.
indentation is not a multiple of four
@@ -85,8 +85,8 @@ def createDaemon(): | |||
# longer a session leader, preventing the daemon from ever acquiring | |||
# a controlling terminal. | |||
pid = os.fork() # Fork a second child. | |||
except OSError, e: | |||
raise Exception, "%s [%d]" % (e.strerror, e.errno) | |||
except OSError as e: |
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.
indentation is not a multiple of four
except OSError, e: | ||
raise Exception, "%s [%d]" % (e.strerror, e.errno) | ||
except OSError as e: | ||
raise Exception("%s [%d]" % (e.strerror, e.errno)) |
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.
indentation is not a multiple of four
@@ -993,7 +993,7 @@ def test_prefix_from_pool(self): | |||
child = s.add_prefix({ 'auth': ad, 'attr': prefix_attr, 'args': args }) | |||
#expected['id'] = child['id'] | |||
#p = s.list_prefix({ 'auth': ad, 'attr': { 'id': child['id'] } })[1] | |||
#self.assertEquals(p, expected) | |||
#self.assertEqual(p, expected) |
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.
block comment should start with '# '
@@ -192,17 +192,17 @@ def test_prefix_add(self): | |||
|
|||
request = requests.post(self.server_url, headers=self.headers, json = attr) | |||
text = request.text | |||
self.assertRegexpMatches(text,"'attr' must be a dict") | |||
self.assertRegex(text,"'attr' must be a dict") |
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.
missing whitespace after ','
|
||
# smart search | ||
res = self._mangle_vrf_result(s.smart_search_vrf({ 'auth': ad, 'query_string': 'forwarding instance' })) | ||
self.assertEquals(res['result'], [ attr, ], 'Smart search result did not match') | ||
self.assertEqual(res['result'], [ attr, ], 'Smart search result did not match') |
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.
line too long (87 > 79 characters)
whitespace after '['
@@ -461,11 +461,11 @@ def test_vrf_add_search(self): | |||
'val2': 'instance 65000' | |||
} | |||
res = self._mangle_vrf_result(s.search_vrf({ 'auth': ad, 'query': q })) | |||
self.assertEquals(res['result'], [ attr, ], 'Search result from regex match did not match') | |||
self.assertEqual(res['result'], [ attr, ], 'Search result from regex match did not match') |
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.
line too long (98 > 79 characters)
whitespace after '['
@@ -354,7 +354,7 @@ def test_vrf_add_list(self): | |||
self.assertEqual(self._mangle_vrf_result(s.list_vrf({ 'auth': ad, 'vrf': { 'id': vrf['id'] } })), [ ref, ]) | |||
|
|||
attr['rt'] = '123:abc' | |||
with self.assertRaisesRegexp(xmlrpclib.Fault, '.'): # TODO: specify exception string | |||
with self.assertRaisesRegex(xmlrpc.client.Fault, '.'): # TODO: specify exception string |
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.
at least two spaces before inline comment
line too long (95 > 79 characters)
|
||
server_url = "http://unittest:[email protected]:1337/XMLRPC" | ||
s = xmlrpclib.Server(server_url, allow_none=1); | ||
s = xmlrpc.client.Server(server_url, allow_none=1); |
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.
statement ends with a semicolon
@@ -25,10 +25,10 @@ | |||
logger.setLevel(logging.DEBUG) | |||
log_format = "%(levelname)-8s %(message)s" | |||
|
|||
import xmlrpclib | |||
import xmlrpc.client |
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.
module level import not at top of file
ae9d5f6
to
5e22443
Compare
Move to newest or replacement libs Fix syntax to 3.6+ Overhaul strings to optimize performance and readability Update build stuff to 3.6 + move docker img to 18.04/bionic
Remove requirements which are built into Python 3.
... as it's not available in Python 3.
There was a special case in the deb package postinst action which set the psql database host to "" if it was configured as localhost. Not sure why this was needed, but "" caused psycopg2 to break in Python 3, could be due to different handling in the config parser... Trying to write nothing instead of "".
To get the tests to pass in Unbuntu 20.04 a few work arounds were needed. Over time the workarounds should be removed, but now I need progress. * Change how nipapd is started when running the apt tests. From some reason the daemon won't start with systemd, so let's just start it manually. Also, it by default tries to drop privileges and then cannot read some postgres certificate in /root. Thus, avoid dropping privileges. And yes, the daemon won't start from systemd without dropping privileges either. * Run the CLI tests over plain-text HTTP. With the newer versions of TLS libs in 20.04, a CN is not enough but a subject alternative name (SAN) is required as well. It's a bit more tricky to generate a cert with SAN, so instead I let the CLI tests run over plain-text HTTP instead. The REST tests pass over TLS (with warnings regarding missing SAN). * As the Python 2 version of NIPAP cannot be installed on Ubuntu 20.04, the upgrade tests are disabled until we can upgrade from a Python 3-based version. * Enabled an "Accident analysis"-step in the CI pipeline. If the test fails it's run and gathers some data for debugging. More debugging ci: Removed debugging ci: Cleanup for starting nipapd manually
Moved the imports in __init__.py in the nipapwww module to be able to import the module during package build without having the dependencies installed.
Fix the PID file handling. Not sure if the changes were introduced in the transition from Python 2 to 3 or later, but changes were needed as the file weren't created on start, PID wasn't visible in the error message when starting two nipapd processes and the PID file wasn't truncated before adding a new PID.
Fixed how selection of number of forks to run was handled.
Try running nipapd from apt again, after fixing handling of forks and PID file.
Updated the WSGI setup guide to instruct the user to install Python 3 version of mod_wsgi.
Surprisingly small changes to make the WWW UI Python 3 compatible.
From some reason the REST tests suddenly starts to fail over HTTPS in the CI environment. There is anoher HTTPS-related problem for XML-RPC related to SNI, but the REST tests previously passed with warnings for this. Disabling HTTPS for REST tests for now.
Revert changes in how config files were opened which was introduced in the Python 3 conversion. The change caused an uncaught exception when trying to open a non-existant config file.
The behavour of configparser seems to have changed, where the Python 2 version returned an empty string when a value in the config file was set to "", two double quotes, and the Python 3 version returns a string containing two double quotes which the PostgreSQL client library dislikes as database host. As this is the default config generated by the Debian install scripts it's probably pretty common, so a workaround was implemented where the string "" results in the previous behaviour.
LGTM, let's go! Some further things to fix before a release can be made, but we need to get this our of the door. |
It's way overdue, but a move to Python 3. Based on the work in #1253.