[issue-997] - More pylint fixes (#2107)

* Update naming and disable false positives

* Rename for naming convention

* Fix linting issues

* Moved pylint to pylintrc

* Moved pylint to pylintrc

* Rename CI

* Check sonar properties

* Fix pull_request_target checkout

.github/workflows/maven-build.yml

@@ -43,6 +43,8 @@ jobs:
 
       - name: Checkout
         uses: actions/checkout@v2
+        with:
+          ref: ${{ github.event.pull_request.head.sha }}
 
       - name: Set up JDK 11
         uses: actions/setup-java@v1

.github/workflows/py-tests.yml

@@ -10,7 +10,7 @@
 #  See the License for the specific language governing permissions and
 #  limitations under the License.
 
-name: py-integration-tests
+name: py-tests
 on:
   push:
     branches: [main]
@@ -24,7 +24,11 @@ jobs:
   run_tests:
     runs-on: ubuntu-latest
     steps:
-    - uses: actions/checkout@v2
+    - name: Checkout
+      uses: actions/checkout@v2
+      with:
+        ref: ${{ github.event.pull_request.head.sha }}
+
     - name: Set up JDK 11
       uses: actions/setup-java@v2
       with:
@@ -68,6 +72,9 @@ jobs:
         echo "sonar.pullrequest.base=origin/main" >> ingestion/sonar-project.properties
         echo "sonar.pullrequest.provider=github" >> ingestion/sonar-project.properties
 
+        # Validate new properties
+        cat ingestion/sonar-project.properties
+
         make token=${{ secrets.INGESTION_SONAR_SECRET }} sonar_ingestion
 
     - name: Run Sonar

.pylintrc

@@ -1,11 +1,17 @@
 [BASIC]
 # W1203: logging-fstring-interpolation - f-string brings better readability and unifies style
 # W1202: logging-format-interpolation - lazy formatting in logging functions
-disable=W1203,W1202
+# R0903: too-few-public-methods - False negatives in pydantic classes
+# W0707: raise-missing-from - Tends to be a false positive as exception are closely encapsulated
+disable=W1203,W1202,R0903,W0707
+
 docstring-min-length=20
 max-args=7
 max-attributes=12
 
+# usual typevar naming
+good-names=T,C
+
 [MASTER]
 fail-under=6.0
 init-hook='from pylint.config import find_pylintrc; import os, sys; sys.path.append(os.path.dirname(find_pylintrc()))'

ingestion/src/metadata/ingestion/ometa/auth_provider.py

@@ -8,6 +8,9 @@
 #  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 #  See the License for the specific language governing permissions and
 #  limitations under the License.
+"""
+Interface definition for an Auth provider
+"""
 
 from abc import ABCMeta, abstractmethod
 from dataclasses import dataclass

ingestion/src/metadata/ingestion/ometa/client.py

@@ -8,10 +8,11 @@
 #  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 #  See the License for the specific language governing permissions and
 #  limitations under the License.
-
+"""
+Python API REST wrapper and helpers
+"""
 import logging
 import time
-from enum import Enum
 from typing import List, Optional
 
 import requests
@@ -48,22 +49,21 @@ class APIError(Exception):
         if http_error is not None and hasattr(http_error, "response"):
             return http_error.response.status_code
 
+        return None
+
     @property
     def request(self):
         if self._http_error is not None:
             return self._http_error.request
 
+        return None
+
     @property
     def response(self):
         if self._http_error is not None:
             return self._http_error.response
 
-
+        return None
-class TimeFrame(Enum):
-    Day = "1Day"
-    Hour = "1Hour"
-    Minute = "1Min"
-    Sec = "1Sec"
 
 
 class ClientConfig(ConfigModel):
@@ -83,7 +83,12 @@ class ClientConfig(ConfigModel):
     allow_redirects: Optional[bool] = False
 
 
-class REST(object):
+class REST:
+    """
+    REST client wrapper to manage requests with
+    retries, auth and error handling.
+    """
+
     def __init__(self, config: ClientConfig):
         self.config = config
         self._base_url: URL = URL(self.config.base_url)

ingestion/src/metadata/ingestion/ometa/credentials.py

@@ -8,6 +8,10 @@
 #  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 #  See the License for the specific language governing permissions and
 #  limitations under the License.
+"""
+Helper methods to handle creds retrieval
+for the OpenMetadata Python API
+"""
 
 import os
 from typing import Tuple
@@ -23,17 +27,17 @@ class URL(str):
         note: we use *value and v0 to allow an empty URL string
         """
         if value:
-            v0 = value[0]
+            url = value[0]
-            if not (isinstance(v0, str) or isinstance(v0, URL)):
+            if not isinstance(url, (URL, str)):
-                raise TypeError(f'Unexpected type for URL: "{type(v0)}"')
+                raise TypeError(f'Unexpected type for URL: "{type(url)}"')
             if not (
-                v0.startswith("http://")
+                url.startswith("http://")
-                or v0.startswith("https://")
+                or url.startswith("https://")
-                or v0.startswith("ws://")
+                or url.startswith("ws://")
-                or v0.startswith("wss://")
+                or url.startswith("wss://")
             ):
                 raise ValueError(
-                    f'Passed string value "{v0}" is not an'
+                    f'Passed string value "{url}" is not an'
                     f' "http*://" or "ws*://" URL'
                 )
         return str.__new__(cls, *value)
@@ -55,9 +59,10 @@ class DATE(str):
             )
         try:
             dateutil.parser.parse(value)
-        except Exception as e:
+        except Exception as exc:
-            msg = f"{value} is not a valid date string: {e}"
+            msg = f"{value} is not a valid date string: {exc}"
             raise Exception(msg)
+
         return str.__new__(cls, value)
 
 
@@ -69,10 +74,11 @@ class FLOAT(str):
     """
 
     def __new__(cls, value):
-        if isinstance(value, float) or isinstance(value, int):
+        if isinstance(value, (float, int)):
             return value
         if isinstance(value, str):
             return float(value.strip())
+
         raise ValueError(f'Unexpected float format "{value}"')
 
 

ingestion/src/metadata/ingestion/ometa/mixins/lineage_mixin.py

@@ -109,6 +109,7 @@ class OMetaLineageMixin(Generic[T]):
             return res
         except APIError as err:
             logger.error(
-                f"Error {err.status_code} trying to GET linage for {entity.__class__.__name__} and {path}"
+                f"Error {err.status_code} trying to GET linage for"
+                + f"{entity.__class__.__name__} and {path}"
             )
             return None

ingestion/src/metadata/ingestion/ometa/mixins/mlmodel_mixin.py

@@ -11,7 +11,7 @@ from metadata.generated.schema.entity.data.mlmodel import MlModel
 from metadata.generated.schema.type.entityLineage import EntitiesEdge
 from metadata.generated.schema.type.entityReference import EntityReference
 from metadata.ingestion.ometa.client import REST
-from metadata.ingestion.ometa.mixins.lineageMixin import OMetaLineageMixin
+from metadata.ingestion.ometa.mixins.lineage_mixin import OMetaLineageMixin
 
 logger = logging.getLogger(__name__)
 

ingestion/src/metadata/ingestion/ometa/mixins/table_mixin.py

@@ -36,7 +36,7 @@ class OMetaTableMixin:
         :param table: Table Entity to update
         :param location: Location Entity to add
         """
-        resp = self.client.put(
+        self.client.put(
             f"{self.get_suffix(Table)}/{table.id.__root__}/location",
             data=str(location.id.__root__),
         )

ingestion/src/metadata/ingestion/ometa/mixins/version_mixin.py

@@ -1,4 +1,3 @@
-# pylint: disable=invalid-name
 """
 Mixin class containing entity versioning specific methods
 
@@ -15,7 +14,7 @@ from metadata.generated.schema.type import basic
 from metadata.generated.schema.type.entityHistory import EntityVersionHistory
 from metadata.ingestion.ometa.client import REST
 
-T = TypeVar("T", bound=BaseModel)  # pylint: disable=invalid-name
+T = TypeVar("T", bound=BaseModel)
 logger = logging.getLogger(__name__)
 
 

ingestion/src/metadata/ingestion/ometa/ometa_api.py

@@ -8,6 +8,12 @@
 #  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 #  See the License for the specific language governing permissions and
 #  limitations under the License.
+"""
+OpenMetadata is the high level Python API that serves as a wrapper
+for the metadata-server API. It is based on the generated pydantic
+models from the JSON schemas and provides a typed approach to
+working with OpenMetadata entities.
+"""
 
 import logging
 from typing import Generic, List, Optional, Type, TypeVar, Union, get_args
@@ -38,10 +44,9 @@ from metadata.generated.schema.type import basic
 from metadata.generated.schema.type.entityHistory import EntityVersionHistory
 from metadata.ingestion.ometa.auth_provider import AuthenticationProvider
 from metadata.ingestion.ometa.client import REST, APIError, ClientConfig
-from metadata.ingestion.ometa.mixins.lineageMixin import OMetaLineageMixin
+from metadata.ingestion.ometa.mixins.mlmodel_mixin import OMetaMlModelMixin
-from metadata.ingestion.ometa.mixins.mlModelMixin import OMetaMlModelMixin
+from metadata.ingestion.ometa.mixins.table_mixin import OMetaTableMixin
-from metadata.ingestion.ometa.mixins.tableMixin import OMetaTableMixin
+from metadata.ingestion.ometa.mixins.version_mixin import OMetaVersionMixin
-from metadata.ingestion.ometa.mixins.versionMixin import OMetaVersionMixin
 from metadata.ingestion.ometa.openmetadata_rest import (
     Auth0AuthenticationProvider,
     GoogleAuthenticationProvider,
@@ -126,13 +131,16 @@ class OpenMetadata(
         self.client = REST(client_config)
         self._use_raw_data = raw_data
 
-    def get_suffix(self, entity: Type[T]) -> str:
+    def get_suffix(self, entity: Type[T]) -> str:  # pylint: disable=R0911,R0912
         """
         Given an entity Type from the generated sources,
         return the endpoint to run requests.
 
         Might be interesting to follow a more strict
         and type-checked approach
+
+        Disabled pylint R0911: too-many-return-statements
+        Disabled pylint R0912: too-many-branches
         """
 
         # Entity Schemas
@@ -411,7 +419,7 @@ class OpenMetadata(
 
         if self._use_raw_data:
             return resp
-        else:
+
         entities = [entity(**t) for t in resp["data"]]
         total = resp["paging"]["total"]
         after = resp["paging"]["after"] if "after" in resp["paging"] else None
@@ -430,7 +438,7 @@ class OpenMetadata(
 
         if self._use_raw_data:
             return resp
-        else:
+
         return EntityVersionHistory(**resp)
 
     def list_services(self, entity: Type[T]) -> List[EntityList[T]]:
@@ -441,7 +449,7 @@ class OpenMetadata(
         resp = self.client.get(self.get_suffix(entity))
         if self._use_raw_data:
             return resp
-        else:
+
         return [entity(**p) for p in resp["data"]]
 
     def delete(self, entity: Type[T], entity_id: Union[str, basic.Uuid]) -> None:

ingestion/src/metadata/ingestion/ometa/openmetadata_rest.py

@@ -8,7 +8,10 @@
 #  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 #  See the License for the specific language governing permissions and
 #  limitations under the License.
-
+"""
+Helper classes to model OpenMetadata Entities,
+server configuration and auth.
+"""
 import http.client
 import json
 import logging
@@ -110,6 +113,10 @@ class GoogleAuthenticationProvider(AuthenticationProvider):
 
 
 class OktaAuthenticationProvider(AuthenticationProvider):
+    """
+    Prepare the Json Web Token for Okta auth
+    """
+
     def __init__(self, config: MetadataServerConfig):
         self.config = config
 
@@ -118,9 +125,9 @@ class OktaAuthenticationProvider(AuthenticationProvider):
         return cls(config)
 
     def auth_token(self) -> str:
-        from okta.jwt import JWT
+        from okta.jwt import JWT  # pylint: disable=import-outside-toplevel
 
-        my_pem, my_jwk = JWT.get_PEM_JWK(self.config.private_key)
+        _, my_jwk = JWT.get_PEM_JWK(self.config.private_key)
         claims = {
             "sub": self.config.client_id,
             "iat": time.time(),

ingestion/src/metadata/ingestion/ometa/superset_rest.py

@@ -8,7 +8,9 @@
 #  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 #  See the License for the specific language governing permissions and
 #  limitations under the License.
-
+"""
+REST Auth & Client for Apache Superset
+"""
 import json
 import logging
 from typing import Optional
@@ -33,6 +35,10 @@ class SupersetConfig(ConfigModel):
 
 
 class SupersetAuthenticationProvider(AuthenticationProvider):
+    """
+    Handle SuperSet Auth
+    """
+
     def __init__(self, config: SupersetConfig):
         self.config = config
         client_config = ClientConfig(base_url=config.url, api_version="api/v1")
@@ -57,7 +63,11 @@ class SupersetAuthenticationProvider(AuthenticationProvider):
         return json.dumps(auth_request)
 
 
-class SupersetAPIClient(object):
+class SupersetAPIClient:
+    """
+    Superset client wrapper using the REST helper class
+    """
+
     client: REST
     _auth_provider: AuthenticationProvider
 
@@ -74,7 +84,7 @@ class SupersetAPIClient(object):
         self.client = REST(client_config)
 
     def fetch_total_dashboards(self) -> int:
-        response = self.client.get(f"/dashboard?q=(page:0,page_size:1)")
+        response = self.client.get("/dashboard?q=(page:0,page_size:1)")
         return response.get("count") or 0
 
     def fetch_dashboards(self, current_page: int, page_size: int):
@@ -84,7 +94,7 @@ class SupersetAPIClient(object):
         return response
 
     def fetch_total_charts(self) -> int:
-        response = self.client.get(f"/chart?q=(page:0,page_size:1)")
+        response = self.client.get("/chart?q=(page:0,page_size:1)")
         return response.get("count") or 0
 
     def fetch_charts(self, current_page: int, page_size: int):