Enable pylint
This is a follow-up from [1] which underlined that the code lacks even basic coverage as it allow
us to introduce a costly regression which could have being detected by pylint no-member test.
- enables pylint with most checks temporary disabled, so we have time
to address them gradually
- fixes few minor issues reported by the tool
- adds skips for some known "no-member" errors but avoids adding it to the exclude list, as this
in order to prevent further regressions.
- once landed we can easily address the temporary disabled errors, one by one. In fact this is could
prove as a very good learning experience for newer team members. They can start by removing on
random exclusion and fixing it. As a hint: leave the missing docstrings for the end, some problems
are more important to fix first.
1: https://review.opendev.org/c/openstack/tripleo-common/+/762892/6/tripleo_common/image/builder/buildah.py
Change-Id: I10ab0cbfbaab77b9208e9a5d74d59eb041cb16ee
(cherry picked from commit 13ac87ba36
)
This commit is contained in:
parent
3521d6ce49
commit
efa167ddfa
|
@ -1,7 +1,7 @@
|
||||||
---
|
---
|
||||||
repos:
|
repos:
|
||||||
- repo: https://github.com/pre-commit/pre-commit-hooks
|
- repo: https://github.com/pre-commit/pre-commit-hooks
|
||||||
rev: v2.5.0
|
rev: v3.4.0
|
||||||
hooks:
|
hooks:
|
||||||
- id: trailing-whitespace
|
- id: trailing-whitespace
|
||||||
- id: mixed-line-ending
|
- id: mixed-line-ending
|
||||||
|
@ -12,7 +12,7 @@ repos:
|
||||||
- id: check-yaml
|
- id: check-yaml
|
||||||
files: .*\.(yaml|yml)$
|
files: .*\.(yaml|yml)$
|
||||||
- repo: https://gitlab.com/pycqa/flake8.git
|
- repo: https://gitlab.com/pycqa/flake8.git
|
||||||
rev: 3.8.1
|
rev: 3.8.4
|
||||||
hooks:
|
hooks:
|
||||||
- id: flake8
|
- id: flake8
|
||||||
- repo: https://github.com/openstack-dev/bashate.git
|
- repo: https://github.com/openstack-dev/bashate.git
|
||||||
|
@ -27,3 +27,7 @@ repos:
|
||||||
# E040: Syntax error determined using `bash -n` (as many scripts
|
# E040: Syntax error determined using `bash -n` (as many scripts
|
||||||
# use jinja templating, this will often fail and the syntax
|
# use jinja templating, this will often fail and the syntax
|
||||||
# error will be discovered in execution anyway)
|
# error will be discovered in execution anyway)
|
||||||
|
- repo: https://github.com/PyCQA/pylint
|
||||||
|
rev: pylint-2.6.0
|
||||||
|
hooks:
|
||||||
|
- id: pylint
|
||||||
|
|
|
@ -0,0 +1,66 @@
|
||||||
|
[MESSAGES CONTROL]
|
||||||
|
|
||||||
|
disable =
|
||||||
|
# TODO(ssbarnea): remove temporary skips adding during initial adoption:
|
||||||
|
arguments-differ,
|
||||||
|
attribute-defined-outside-init,
|
||||||
|
broad-except,
|
||||||
|
consider-iterating-dictionary,
|
||||||
|
consider-merging-isinstance,
|
||||||
|
consider-using-dict-comprehension,
|
||||||
|
consider-using-in,
|
||||||
|
consider-using-set-comprehension,
|
||||||
|
dangerous-default-value,
|
||||||
|
duplicate-code,
|
||||||
|
fixme,
|
||||||
|
global-statement,
|
||||||
|
import-error,
|
||||||
|
inconsistent-return-statements,
|
||||||
|
invalid-name,
|
||||||
|
logging-format-interpolation,
|
||||||
|
logging-not-lazy,
|
||||||
|
lost-exception,
|
||||||
|
missing-class-docstring,
|
||||||
|
missing-function-docstring,
|
||||||
|
missing-module-docstring,
|
||||||
|
no-else-break,
|
||||||
|
no-else-continue,
|
||||||
|
no-else-raise,
|
||||||
|
no-else-return,
|
||||||
|
no-self-use,
|
||||||
|
no-value-for-parameter,
|
||||||
|
protected-access,
|
||||||
|
raise-missing-from,
|
||||||
|
redefined-argument-from-local,
|
||||||
|
redefined-builtin,
|
||||||
|
redefined-outer-name,
|
||||||
|
simplifiable-if-statement,
|
||||||
|
super-init-not-called,
|
||||||
|
super-with-arguments,
|
||||||
|
superfluous-parens,
|
||||||
|
too-few-public-methods,
|
||||||
|
too-many-ancestors,
|
||||||
|
too-many-arguments,
|
||||||
|
too-many-branches,
|
||||||
|
too-many-instance-attributes,
|
||||||
|
too-many-lines,
|
||||||
|
too-many-locals,
|
||||||
|
too-many-nested-blocks,
|
||||||
|
too-many-public-methods,
|
||||||
|
too-many-return-statements,
|
||||||
|
too-many-statements,
|
||||||
|
try-except-raise,
|
||||||
|
unidiomatic-typecheck,
|
||||||
|
unnecessary-comprehension,
|
||||||
|
unnecessary-pass,
|
||||||
|
unsubscriptable-object,
|
||||||
|
unused-argument,
|
||||||
|
unused-variable,
|
||||||
|
useless-else-on-loop,
|
||||||
|
useless-object-inheritance,
|
||||||
|
useless-super-delegation,
|
||||||
|
wrong-import-order,
|
||||||
|
wrong-import-position
|
||||||
|
|
||||||
|
[REPORTS]
|
||||||
|
output-format = colorized
|
|
@ -72,10 +72,10 @@ def rm_container(name):
|
||||||
if rc != 0:
|
if rc != 0:
|
||||||
|
|
||||||
if 'No such container' in cmd_stderr:
|
if 'No such container' in cmd_stderr:
|
||||||
log.warn('Container that does not exist cannot be deleted: '
|
log.warning('Container that does not exist cannot be deleted: '
|
||||||
'%s' % name)
|
'%s', name)
|
||||||
else:
|
else:
|
||||||
log.error('Error removing container: %s' % name)
|
log.error('Error removing container: %s', name)
|
||||||
log.error(cmd_stderr)
|
log.error(cmd_stderr)
|
||||||
raise RuntimeError(cmd_stdout, cmd_stderr, rc)
|
raise RuntimeError(cmd_stdout, cmd_stderr, rc)
|
||||||
|
|
||||||
|
|
1
setup.py
1
setup.py
|
@ -20,6 +20,7 @@ import setuptools
|
||||||
# setuptools if some other modules registered functions in `atexit`.
|
# setuptools if some other modules registered functions in `atexit`.
|
||||||
# solution from: http://bugs.python.org/issue15881#msg170215
|
# solution from: http://bugs.python.org/issue15881#msg170215
|
||||||
try:
|
try:
|
||||||
|
# pylint: disable=unused-import
|
||||||
import multiprocessing # noqa
|
import multiprocessing # noqa
|
||||||
except ImportError:
|
except ImportError:
|
||||||
pass
|
pass
|
||||||
|
|
|
@ -167,7 +167,7 @@ def write_default_ansible_cfg(work_dir,
|
||||||
sio_cfg.write(override_ansible_cfg)
|
sio_cfg.write(override_ansible_cfg)
|
||||||
sio_cfg.seek(0)
|
sio_cfg.seek(0)
|
||||||
if sys.version_info[0] < 3:
|
if sys.version_info[0] < 3:
|
||||||
config.readfp(sio_cfg)
|
config.readfp(sio_cfg) # pylint: disable=deprecated-method
|
||||||
else:
|
else:
|
||||||
config.read_file(sio_cfg)
|
config.read_file(sio_cfg)
|
||||||
sio_cfg.close()
|
sio_cfg.close()
|
||||||
|
|
|
@ -113,7 +113,7 @@ class OrchestrationDeployAction(base.TripleOAction):
|
||||||
swift_client.delete_object(container_name, object_name)
|
swift_client.delete_object(container_name, object_name)
|
||||||
swift_client.delete_container(container_name)
|
swift_client.delete_container(container_name)
|
||||||
except Exception as err:
|
except Exception as err:
|
||||||
LOG.error("Error cleaning up heat deployment resources.", err)
|
LOG.error("Error cleaning up heat deployment resources: %s", err)
|
||||||
|
|
||||||
error = None
|
error = None
|
||||||
if not body:
|
if not body:
|
||||||
|
|
|
@ -104,7 +104,7 @@ class CreateDatabaseBackup(base.Action):
|
||||||
return actions.Result(error={"msg": six.text_type(msg)})
|
return actions.Result(error={"msg": six.text_type(msg)})
|
||||||
lockfile = open(pid_file, 'w')
|
lockfile = open(pid_file, 'w')
|
||||||
lockfile.write("%s\n" % os.getpid())
|
lockfile.write("%s\n" % os.getpid())
|
||||||
lockfile.close
|
lockfile.close()
|
||||||
|
|
||||||
# Backup all databases with nice and ionice just not to create
|
# Backup all databases with nice and ionice just not to create
|
||||||
# a huge load on undercloud. Output will be redirected to mysqldump
|
# a huge load on undercloud. Output will be redirected to mysqldump
|
||||||
|
|
|
@ -39,6 +39,7 @@ class TripleoCommonException(Exception):
|
||||||
|
|
||||||
def __init__(self, **kwargs):
|
def __init__(self, **kwargs):
|
||||||
self.kwargs = kwargs
|
self.kwargs = kwargs
|
||||||
|
self.msg_fmt = self.message
|
||||||
|
|
||||||
try:
|
try:
|
||||||
self.message = self.msg_fmt % kwargs
|
self.message = self.msg_fmt % kwargs
|
||||||
|
|
|
@ -576,7 +576,7 @@ class ImageUploadManager(BaseImageManager):
|
||||||
|
|
||||||
|
|
||||||
class BaseImageUploader(object):
|
class BaseImageUploader(object):
|
||||||
|
lock = None
|
||||||
mirrors = {}
|
mirrors = {}
|
||||||
insecure_registries = set()
|
insecure_registries = set()
|
||||||
no_verify_registries = set(NO_VERIFY_REGISTRIES)
|
no_verify_registries = set(NO_VERIFY_REGISTRIES)
|
||||||
|
@ -1224,11 +1224,13 @@ class BaseImageUploader(object):
|
||||||
name = target_image_url.path.split(':')[0][1:]
|
name = target_image_url.path.split(':')[0][1:]
|
||||||
export = netloc in cls.export_registries
|
export = netloc in cls.export_registries
|
||||||
if export:
|
if export:
|
||||||
|
# pylint: disable=no-member
|
||||||
linked_layers = image_export.cross_repo_mount(
|
linked_layers = image_export.cross_repo_mount(
|
||||||
target_image_url, image_layers, source_layers,
|
target_image_url, image_layers, source_layers,
|
||||||
uploaded_layers=cls._global_view_proxy())
|
uploaded_layers=cls._global_view_proxy())
|
||||||
# track linked layers globally for future references
|
# track linked layers globally for future references
|
||||||
for layer, info in linked_layers.items():
|
for layer, info in linked_layers.items():
|
||||||
|
# pylint: disable=no-member
|
||||||
cls._track_uploaded_layers(
|
cls._track_uploaded_layers(
|
||||||
layer, known_path=info['known_path'],
|
layer, known_path=info['known_path'],
|
||||||
image_ref=info['ref_image'], scope='local')
|
image_ref=info['ref_image'], scope='local')
|
||||||
|
@ -1241,6 +1243,7 @@ class BaseImageUploader(object):
|
||||||
url = '%s://%s/v2/%s/blobs/uploads/' % (scheme, netloc, name)
|
url = '%s://%s/v2/%s/blobs/uploads/' % (scheme, netloc, name)
|
||||||
|
|
||||||
for layer in source_layers:
|
for layer in source_layers:
|
||||||
|
# pylint: disable=no-member
|
||||||
known_path, existing_name = image_utils.uploaded_layers_details(
|
known_path, existing_name = image_utils.uploaded_layers_details(
|
||||||
cls._global_view_proxy(), layer, scope='remote')
|
cls._global_view_proxy(), layer, scope='remote')
|
||||||
if layer not in image_layers and not existing_name:
|
if layer not in image_layers and not existing_name:
|
||||||
|
|
|
@ -627,6 +627,7 @@ class TestUndeployInstance(base.TestCase):
|
||||||
def test_success(self, mock_pr):
|
def test_success(self, mock_pr):
|
||||||
pr = mock_pr.return_value
|
pr = mock_pr.return_value
|
||||||
action = baremetal_deploy.UndeployInstanceAction('inst1')
|
action = baremetal_deploy.UndeployInstanceAction('inst1')
|
||||||
|
# pylint: disable=assignment-from-none
|
||||||
result = action.run(mock.Mock())
|
result = action.run(mock.Mock())
|
||||||
self.assertIsNone(result)
|
self.assertIsNone(result)
|
||||||
|
|
||||||
|
@ -638,6 +639,7 @@ class TestUndeployInstance(base.TestCase):
|
||||||
pr = mock_pr.return_value
|
pr = mock_pr.return_value
|
||||||
pr.show_instance.side_effect = RuntimeError('not found')
|
pr.show_instance.side_effect = RuntimeError('not found')
|
||||||
action = baremetal_deploy.UndeployInstanceAction('inst1')
|
action = baremetal_deploy.UndeployInstanceAction('inst1')
|
||||||
|
# pylint: disable=assignment-from-none
|
||||||
result = action.run(mock.Mock())
|
result = action.run(mock.Mock())
|
||||||
self.assertIsNone(result)
|
self.assertIsNone(result)
|
||||||
|
|
||||||
|
|
|
@ -420,18 +420,18 @@ class ProcessTemplatesActionTest(base.TestCase):
|
||||||
self.assertTrue({'name': ['puppet/controller-role.yaml']} ==
|
self.assertTrue({'name': ['puppet/controller-role.yaml']} ==
|
||||||
action._get_j2_excludes_file(mock_ctx))
|
action._get_j2_excludes_file(mock_ctx))
|
||||||
|
|
||||||
def return_multiple_files(*args):
|
def return_multiple_files2(*args):
|
||||||
if args[1] == constants.OVERCLOUD_J2_EXCLUDES:
|
if args[1] == constants.OVERCLOUD_J2_EXCLUDES:
|
||||||
return ['', J2_EXCLUDES_EMPTY_LIST]
|
return ['', J2_EXCLUDES_EMPTY_LIST]
|
||||||
swift.get_object = mock.MagicMock(side_effect=return_multiple_files)
|
swift.get_object = mock.MagicMock(side_effect=return_multiple_files2)
|
||||||
# Test - J2 exclude file with no template to exlude
|
# Test - J2 exclude file with no template to exlude
|
||||||
action = templates.ProcessTemplatesAction()
|
action = templates.ProcessTemplatesAction()
|
||||||
self.assertTrue({'name': []} == action._get_j2_excludes_file(mock_ctx))
|
self.assertTrue({'name': []} == action._get_j2_excludes_file(mock_ctx))
|
||||||
|
|
||||||
def return_multiple_files(*args):
|
def return_multiple_files3(*args):
|
||||||
if args[1] == constants.OVERCLOUD_J2_EXCLUDES:
|
if args[1] == constants.OVERCLOUD_J2_EXCLUDES:
|
||||||
return ['', J2_EXCLUDES_EMPTY_FILE]
|
return ['', J2_EXCLUDES_EMPTY_FILE]
|
||||||
swift.get_object = mock.MagicMock(side_effect=return_multiple_files)
|
swift.get_object = mock.MagicMock(side_effect=return_multiple_files3)
|
||||||
# Test - J2 exclude file empty
|
# Test - J2 exclude file empty
|
||||||
action = templates.ProcessTemplatesAction()
|
action = templates.ProcessTemplatesAction()
|
||||||
self.assertTrue({'name': []} == action._get_j2_excludes_file(mock_ctx))
|
self.assertTrue({'name': []} == action._get_j2_excludes_file(mock_ctx))
|
||||||
|
|
|
@ -490,6 +490,7 @@ class TestBaseImageUploader(base.TestCase):
|
||||||
super(TestBaseImageUploader, self).setUp()
|
super(TestBaseImageUploader, self).setUp()
|
||||||
self.uploader = image_uploader.BaseImageUploader()
|
self.uploader = image_uploader.BaseImageUploader()
|
||||||
self.uploader.init_registries_cache()
|
self.uploader.init_registries_cache()
|
||||||
|
# pylint: disable=no-member
|
||||||
self.uploader._inspect.retry.sleep = mock.Mock()
|
self.uploader._inspect.retry.sleep = mock.Mock()
|
||||||
self.requests = self.useFixture(rm_fixture.Fixture())
|
self.requests = self.useFixture(rm_fixture.Fixture())
|
||||||
|
|
||||||
|
@ -1288,7 +1289,9 @@ class TestSkopeoImageUploader(base.TestCase):
|
||||||
def setUp(self):
|
def setUp(self):
|
||||||
super(TestSkopeoImageUploader, self).setUp()
|
super(TestSkopeoImageUploader, self).setUp()
|
||||||
self.uploader = image_uploader.SkopeoImageUploader()
|
self.uploader = image_uploader.SkopeoImageUploader()
|
||||||
|
# pylint: disable=no-member
|
||||||
self.uploader._copy.retry.sleep = mock.Mock()
|
self.uploader._copy.retry.sleep = mock.Mock()
|
||||||
|
# pylint: disable=no-member
|
||||||
self.uploader._inspect.retry.sleep = mock.Mock()
|
self.uploader._inspect.retry.sleep = mock.Mock()
|
||||||
|
|
||||||
@mock.patch('tripleo_common.image.image_uploader.'
|
@mock.patch('tripleo_common.image.image_uploader.'
|
||||||
|
@ -1532,6 +1535,7 @@ class TestSkopeoImageUploader(base.TestCase):
|
||||||
|
|
||||||
class TestPythonImageUploader(base.TestCase):
|
class TestPythonImageUploader(base.TestCase):
|
||||||
|
|
||||||
|
# pylint: disable=no-member
|
||||||
def setUp(self):
|
def setUp(self):
|
||||||
super(TestPythonImageUploader, self).setUp()
|
super(TestPythonImageUploader, self).setUp()
|
||||||
self.uploader = image_uploader.PythonImageUploader()
|
self.uploader = image_uploader.PythonImageUploader()
|
||||||
|
|
|
@ -15,10 +15,13 @@
|
||||||
|
|
||||||
class BaseLock(object):
|
class BaseLock(object):
|
||||||
def get_lock(self):
|
def get_lock(self):
|
||||||
|
# pylint: disable=no-member
|
||||||
return self._lock
|
return self._lock
|
||||||
|
|
||||||
def objects(self):
|
def objects(self):
|
||||||
|
# pylint: disable=no-member
|
||||||
return self._objects
|
return self._objects
|
||||||
|
|
||||||
def sessions(self):
|
def sessions(self):
|
||||||
|
# pylint: disable=no-member
|
||||||
return self._sessions
|
return self._sessions
|
||||||
|
|
|
@ -26,6 +26,8 @@ class ProcessLock(base.BaseLock):
|
||||||
_global_view = _mgr.dict()
|
_global_view = _mgr.dict()
|
||||||
|
|
||||||
def __init__(self):
|
def __init__(self):
|
||||||
|
# https://github.com/PyCQA/pylint/issues/3313
|
||||||
|
# pylint: disable=no-member
|
||||||
self._lock = self._mgr.Lock()
|
self._lock = self._mgr.Lock()
|
||||||
self._objects = self._mgr.list()
|
self._objects = self._mgr.list()
|
||||||
self._sessions = self._mgr.dict()
|
self._sessions = self._mgr.dict()
|
||||||
|
|
Loading…
Reference in New Issue