From 74e9038b9230851447de9dab1c6f62343f3f51ef Mon Sep 17 00:00:00 2001 From: Pere Miquel Brull Date: Fri, 7 Jan 2022 19:50:53 +0100 Subject: [PATCH] [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 | 2 ++ .github/workflows/py-tests.yml | 11 ++++-- .pylintrc | 8 ++++- .../metadata/ingestion/ometa/auth_provider.py | 3 ++ .../src/metadata/ingestion/ometa/client.py | 23 +++++++----- .../metadata/ingestion/ometa/credentials.py | 28 +++++++++------ .../{lineageMixin.py => lineage_mixin.py} | 3 +- .../{mlModelMixin.py => mlmodel_mixin.py} | 2 +- .../mixins/{tableMixin.py => table_mixin.py} | 2 +- .../{versionMixin.py => version_mixin.py} | 3 +- .../src/metadata/ingestion/ometa/ometa_api.py | 36 +++++++++++-------- .../ingestion/ometa/openmetadata_rest.py | 13 +++++-- .../metadata/ingestion/ometa/superset_rest.py | 18 +++++++--- 13 files changed, 103 insertions(+), 49 deletions(-) rename ingestion/src/metadata/ingestion/ometa/mixins/{lineageMixin.py => lineage_mixin.py} (98%) rename ingestion/src/metadata/ingestion/ometa/mixins/{mlModelMixin.py => mlmodel_mixin.py} (95%) rename ingestion/src/metadata/ingestion/ometa/mixins/{tableMixin.py => table_mixin.py} (99%) rename ingestion/src/metadata/ingestion/ometa/mixins/{versionMixin.py => version_mixin.py} (96%) diff --git a/.github/workflows/maven-build.yml b/.github/workflows/maven-build.yml index ebac00a0b97..19e402a6726 100644 --- a/.github/workflows/maven-build.yml +++ b/.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 diff --git a/.github/workflows/py-tests.yml b/.github/workflows/py-tests.yml index 4e2cff764b6..227ba5b7882 100644 --- a/.github/workflows/py-tests.yml +++ b/.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 diff --git a/.pylintrc b/.pylintrc index 2e99493ce45..207fe7a4810 100644 --- a/.pylintrc +++ b/.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()))' diff --git a/ingestion/src/metadata/ingestion/ometa/auth_provider.py b/ingestion/src/metadata/ingestion/ometa/auth_provider.py index ef98d641a34..331b29ec529 100644 --- a/ingestion/src/metadata/ingestion/ometa/auth_provider.py +++ b/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 diff --git a/ingestion/src/metadata/ingestion/ometa/client.py b/ingestion/src/metadata/ingestion/ometa/client.py index cab7ace9650..4eddfda3af4 100644 --- a/ingestion/src/metadata/ingestion/ometa/client.py +++ b/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 - -class TimeFrame(Enum): - Day = "1Day" - Hour = "1Hour" - Minute = "1Min" - Sec = "1Sec" + return None 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) diff --git a/ingestion/src/metadata/ingestion/ometa/credentials.py b/ingestion/src/metadata/ingestion/ometa/credentials.py index e7194131e95..802021fb118 100644 --- a/ingestion/src/metadata/ingestion/ometa/credentials.py +++ b/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] - if not (isinstance(v0, str) or isinstance(v0, URL)): - raise TypeError(f'Unexpected type for URL: "{type(v0)}"') + url = value[0] + if not isinstance(url, (URL, str)): + raise TypeError(f'Unexpected type for URL: "{type(url)}"') if not ( - v0.startswith("http://") - or v0.startswith("https://") - or v0.startswith("ws://") - or v0.startswith("wss://") + url.startswith("http://") + or url.startswith("https://") + or url.startswith("ws://") + 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: - msg = f"{value} is not a valid date string: {e}" + except Exception as exc: + 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}"') diff --git a/ingestion/src/metadata/ingestion/ometa/mixins/lineageMixin.py b/ingestion/src/metadata/ingestion/ometa/mixins/lineage_mixin.py similarity index 98% rename from ingestion/src/metadata/ingestion/ometa/mixins/lineageMixin.py rename to ingestion/src/metadata/ingestion/ometa/mixins/lineage_mixin.py index 08fe0c0a964..fe3ce4be068 100644 --- a/ingestion/src/metadata/ingestion/ometa/mixins/lineageMixin.py +++ b/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 diff --git a/ingestion/src/metadata/ingestion/ometa/mixins/mlModelMixin.py b/ingestion/src/metadata/ingestion/ometa/mixins/mlmodel_mixin.py similarity index 95% rename from ingestion/src/metadata/ingestion/ometa/mixins/mlModelMixin.py rename to ingestion/src/metadata/ingestion/ometa/mixins/mlmodel_mixin.py index ab18779153f..b6cc543bc28 100644 --- a/ingestion/src/metadata/ingestion/ometa/mixins/mlModelMixin.py +++ b/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__) diff --git a/ingestion/src/metadata/ingestion/ometa/mixins/tableMixin.py b/ingestion/src/metadata/ingestion/ometa/mixins/table_mixin.py similarity index 99% rename from ingestion/src/metadata/ingestion/ometa/mixins/tableMixin.py rename to ingestion/src/metadata/ingestion/ometa/mixins/table_mixin.py index 9188eea7696..b953eb95139 100644 --- a/ingestion/src/metadata/ingestion/ometa/mixins/tableMixin.py +++ b/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__), ) diff --git a/ingestion/src/metadata/ingestion/ometa/mixins/versionMixin.py b/ingestion/src/metadata/ingestion/ometa/mixins/version_mixin.py similarity index 96% rename from ingestion/src/metadata/ingestion/ometa/mixins/versionMixin.py rename to ingestion/src/metadata/ingestion/ometa/mixins/version_mixin.py index 0169e322a0e..cd186591e00 100644 --- a/ingestion/src/metadata/ingestion/ometa/mixins/versionMixin.py +++ b/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__) diff --git a/ingestion/src/metadata/ingestion/ometa/ometa_api.py b/ingestion/src/metadata/ingestion/ometa/ometa_api.py index 0d6bfda6555..6f05df50947 100644 --- a/ingestion/src/metadata/ingestion/ometa/ometa_api.py +++ b/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.mlModelMixin import OMetaMlModelMixin -from metadata.ingestion.ometa.mixins.tableMixin import OMetaTableMixin -from metadata.ingestion.ometa.mixins.versionMixin import OMetaVersionMixin +from metadata.ingestion.ometa.mixins.mlmodel_mixin import OMetaMlModelMixin +from metadata.ingestion.ometa.mixins.table_mixin import OMetaTableMixin +from metadata.ingestion.ometa.mixins.version_mixin 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,11 +419,11 @@ 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 - return EntityList(entities=entities, total=total, after=after) + + entities = [entity(**t) for t in resp["data"]] + total = resp["paging"]["total"] + after = resp["paging"]["after"] if "after" in resp["paging"] else None + return EntityList(entities=entities, total=total, after=after) def list_versions( self, entity_id: Union[str, basic.Uuid], entity: Type[T] @@ -430,8 +438,8 @@ class OpenMetadata( if self._use_raw_data: return resp - else: - return EntityVersionHistory(**resp) + + return EntityVersionHistory(**resp) def list_services(self, entity: Type[T]) -> List[EntityList[T]]: """ @@ -441,8 +449,8 @@ 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"]] + + return [entity(**p) for p in resp["data"]] def delete(self, entity: Type[T], entity_id: Union[str, basic.Uuid]) -> None: self.client.delete(f"{self.get_suffix(entity)}/{self.uuid_to_str(entity_id)}") diff --git a/ingestion/src/metadata/ingestion/ometa/openmetadata_rest.py b/ingestion/src/metadata/ingestion/ometa/openmetadata_rest.py index e86c7c0ea5e..ed286249fa8 100644 --- a/ingestion/src/metadata/ingestion/ometa/openmetadata_rest.py +++ b/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(), diff --git a/ingestion/src/metadata/ingestion/ometa/superset_rest.py b/ingestion/src/metadata/ingestion/ometa/superset_rest.py index 05b24c68e77..17bc0907713 100644 --- a/ingestion/src/metadata/ingestion/ometa/superset_rest.py +++ b/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):