From 088d57870c7621cb4f70f1f097ceb708a0176a53 Mon Sep 17 00:00:00 2001 From: Sorin Sbarnea Date: Wed, 16 Dec 2020 13:58:56 +0000 Subject: [PATCH] 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 13ac87ba36be2d8cddb37cd82d242dcb0a8717f5) --- .pre-commit-config.yaml | 8 ++- .pylintrc | 65 +++++++++++++++++++ tripleo_common/exception.py | 1 + tripleo_common/image/image_uploader.py | 5 +- .../tests/image/test_image_uploader.py | 4 ++ tripleo_common/tests/utils/test_template.py | 8 +-- tripleo_common/utils/locks/base.py | 3 + tripleo_common/utils/locks/processlock.py | 2 + 8 files changed, 89 insertions(+), 7 deletions(-) create mode 100644 .pylintrc diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 5d4af3c66..201d4237d 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -1,7 +1,7 @@ --- repos: - repo: https://github.com/pre-commit/pre-commit-hooks - rev: v2.5.0 + rev: v3.4.0 hooks: - id: trailing-whitespace - id: mixed-line-ending @@ -12,7 +12,7 @@ repos: - id: check-yaml files: .*\.(yaml|yml)$ - repo: https://gitlab.com/pycqa/flake8.git - rev: 3.8.1 + rev: 3.8.4 hooks: - id: flake8 - repo: https://github.com/openstack-dev/bashate.git @@ -27,3 +27,7 @@ repos: # E040: Syntax error determined using `bash -n` (as many scripts # use jinja templating, this will often fail and the syntax # error will be discovered in execution anyway) + - repo: https://github.com/PyCQA/pylint + rev: pylint-2.6.0 + hooks: + - id: pylint diff --git a/.pylintrc b/.pylintrc new file mode 100644 index 000000000..338b361f9 --- /dev/null +++ b/.pylintrc @@ -0,0 +1,65 @@ +[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-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 diff --git a/tripleo_common/exception.py b/tripleo_common/exception.py index 2122df1e9..b9fb670b4 100644 --- a/tripleo_common/exception.py +++ b/tripleo_common/exception.py @@ -39,6 +39,7 @@ class TripleoCommonException(Exception): def __init__(self, **kwargs): self.kwargs = kwargs + self.msg_fmt = self.message try: self.message = self.msg_fmt % kwargs diff --git a/tripleo_common/image/image_uploader.py b/tripleo_common/image/image_uploader.py index b924b0fc6..92a9bb9a3 100644 --- a/tripleo_common/image/image_uploader.py +++ b/tripleo_common/image/image_uploader.py @@ -575,7 +575,7 @@ class ImageUploadManager(BaseImageManager): class BaseImageUploader(object): - + lock = None mirrors = {} insecure_registries = set() no_verify_registries = set(NO_VERIFY_REGISTRIES) @@ -1237,11 +1237,13 @@ class BaseImageUploader(object): name = target_image_url.path.split(':')[0][1:] export = netloc in cls.export_registries if export: + # pylint: disable=no-member linked_layers = image_export.cross_repo_mount( target_image_url, image_layers, source_layers, uploaded_layers=cls._global_view_proxy()) # track linked layers globally for future references for layer, info in linked_layers.items(): + # pylint: disable=no-member cls._track_uploaded_layers( layer, known_path=info['known_path'], image_ref=info['ref_image'], scope='local') @@ -1254,6 +1256,7 @@ class BaseImageUploader(object): url = '%s://%s/v2/%s/blobs/uploads/' % (scheme, netloc, name) for layer in source_layers: + # pylint: disable=no-member known_path, existing_name = image_utils.uploaded_layers_details( cls._global_view_proxy(), layer, scope='remote') if layer not in image_layers and not existing_name: diff --git a/tripleo_common/tests/image/test_image_uploader.py b/tripleo_common/tests/image/test_image_uploader.py index 416bd18c1..6ae1aea2a 100644 --- a/tripleo_common/tests/image/test_image_uploader.py +++ b/tripleo_common/tests/image/test_image_uploader.py @@ -490,6 +490,7 @@ class TestBaseImageUploader(base.TestCase): super(TestBaseImageUploader, self).setUp() self.uploader = image_uploader.BaseImageUploader() self.uploader.init_registries_cache() + # pylint: disable=no-member self.uploader._inspect.retry.sleep = mock.Mock() self.requests = self.useFixture(rm_fixture.Fixture()) @@ -1288,7 +1289,9 @@ class TestSkopeoImageUploader(base.TestCase): def setUp(self): super(TestSkopeoImageUploader, self).setUp() self.uploader = image_uploader.SkopeoImageUploader() + # pylint: disable=no-member self.uploader._copy.retry.sleep = mock.Mock() + # pylint: disable=no-member self.uploader._inspect.retry.sleep = mock.Mock() @mock.patch('tripleo_common.image.image_uploader.' @@ -1534,6 +1537,7 @@ class TestSkopeoImageUploader(base.TestCase): class TestPythonImageUploader(base.TestCase): + # pylint: disable=no-member def setUp(self): super(TestPythonImageUploader, self).setUp() self.uploader = image_uploader.PythonImageUploader() diff --git a/tripleo_common/tests/utils/test_template.py b/tripleo_common/tests/utils/test_template.py index 92bd2c9d2..e1d71a06e 100644 --- a/tripleo_common/tests/utils/test_template.py +++ b/tripleo_common/tests/utils/test_template.py @@ -380,18 +380,18 @@ class ProcessTemplatesTest(base.TestCase): self.assertTrue({'name': ['puppet/controller-role.yaml']} == template_utils.get_j2_excludes_file(swift)) - def return_multiple_files(*args): + def return_multiple_files2(*args): if args[1] == constants.OVERCLOUD_J2_EXCLUDES: 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 self.assertTrue( {'name': []} == template_utils.get_j2_excludes_file(swift)) - def return_multiple_files(*args): + def return_multiple_files3(*args): if args[1] == constants.OVERCLOUD_J2_EXCLUDES: 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 self.assertTrue( {'name': []} == template_utils.get_j2_excludes_file(swift)) diff --git a/tripleo_common/utils/locks/base.py b/tripleo_common/utils/locks/base.py index a9fd96727..fb7f5e5d2 100644 --- a/tripleo_common/utils/locks/base.py +++ b/tripleo_common/utils/locks/base.py @@ -15,10 +15,13 @@ class BaseLock(object): def get_lock(self): + # pylint: disable=no-member return self._lock def objects(self): + # pylint: disable=no-member return self._objects def sessions(self): + # pylint: disable=no-member return self._sessions diff --git a/tripleo_common/utils/locks/processlock.py b/tripleo_common/utils/locks/processlock.py index 26c726b68..c30aee8c5 100644 --- a/tripleo_common/utils/locks/processlock.py +++ b/tripleo_common/utils/locks/processlock.py @@ -26,6 +26,8 @@ class ProcessLock(base.BaseLock): _global_view = _mgr.dict() def __init__(self): + # https://github.com/PyCQA/pylint/issues/3313 + # pylint: disable=no-member self._lock = self._mgr.Lock() self._objects = self._mgr.list() self._sessions = self._mgr.dict()