✨ introduce make lint to apply various code checks (#28)

* introduce pre-commit

* add pre-commit hook

* add pre-commit hook

* add pre-commit hook

* add docs

Author: renardeinside

.gitignore

@@ -133,4 +133,4 @@ cython_debug/
 .idea/
 
 # ruff
-.ruff_cache
+.ruff_cache

Makefile

@@ -0,0 +1,11 @@
+lint:
+	@echo "Linting the project code"
+	black .
+	isort .
+	ruff . --fix
+
+verify:
+	@echo "Verifying the project code"
+	black . --check
+	isort . --check
+	ruff .

README.md

@@ -1,6 +1,8 @@
-# UC Upgrade Utilities
+# UC Migration Toolkit
 
 This repo contains various functions and utilities for UC Upgrade.
+
+
 ## Latest working version and how-to
 
 Please note that current project statis is 🏗️ **WIP**, but we have a minimal set of already working utilities.
@@ -17,18 +19,25 @@ Recommended VM types are:
 **For now please switch to the `v0.0.1` tag in the GitHub to get the latest working version.**
 
 
-## Local setup
+## Local setup and development process
 
 - Install [poetry](https://python-poetry.org/)
 - Run `poetry install` in the project directory
 - Pin your IDE to use the newly created poetry environment
 
-> Please note that you **don't** need to use `poetry` inside notebooks or in the Databricks workspace. 
+> Please note that you **don't** need to use `poetry` inside notebooks or in the Databricks workspace.
 > It's only introduced to simplify local development.
 
+Before running `git push`, don't forget to link your code with:
+
+```shell
+make lint
+```
+
 ### Details of package installation
 
 Since the package itself is managed with `poetry`, to re-use it inside the notebooks we're doing the following:
 
 1. Installing the package dependencies via poetry export
-2. Adding the package itself to the notebook via `sys.path`
+2. Adding the package itself to the notebook via `sys.path`
+

notebooks/common.py

@@ -2,11 +2,11 @@
 from pathlib import Path
 from tempfile import NamedTemporaryFile
 
-from databricks.sdk.runtime import * # noqa: F403
+from databricks.sdk.runtime import *  # noqa: F403
 
 
 def install_uc_upgrade_package():
-    ipython = get_ipython() # noqa: F405
+    ipython = get_ipython()  # noqa: F405
 
     print("Installing poetry for package management")
     ipython.run_line_magic("pip", "install poetry -I")
@@ -19,7 +19,7 @@ def install_uc_upgrade_package():
         print("Saved the requirements to a provided file, installing them with pip")
         ipython.run_line_magic("pip", f"install -r {requirements_file.name} -I")
         print("Requirements installed successfully, restarting Python interpreter")
-        dbutils.library.restartPython() # noqa: F405
+        dbutils.library.restartPython()  # noqa: F405
         print("Python interpreter restarted successfully")
 
     print("Reloading the path-based modules")
@@ -33,7 +33,7 @@ def install_uc_upgrade_package():
 
     print("Verifying that package can be properly loaded")
     try:
-        from uc_upgrade.group_migration import GroupMigration # noqa: F401
+        from uc_upgrade.group_migration import GroupMigration  # noqa: F401
 
         print("Successfully loaded the uc-upgrade package")
     except Exception as e:

uc_upgrade/group_migration.py

@@ -1,5 +1,6 @@
 import concurrent.futures
 import json
+import logging
 import math
 import time
 from typing import List
@@ -8,7 +9,6 @@
 from pyspark.sql import session
 from pyspark.sql.functions import array_contains, col, collect_set, lit
 from pyspark.sql.types import MapType, StringType, StructField, StructType
-import logging
 
 # Initialize logger
 logger = logging.getLogger(__name__)
@@ -82,15 +82,17 @@ def __init__(
         spark.sql(f"drop table if exists {self.inventoryTableName+'TableACL'}")
 
         # Check if we should automatically generate list, and do it immediately.
-        # Implementers Note: Could change this section to a lazy calculation by setting groupL to nil or some sentinel value and adding checks before use.
+        # Implementers Note: Could change this section to a lazy calculation
+        # by setting groupL to nil or some sentinel value and adding checks before use.
         res = requests.get(f"{self.workspace_url}/api/2.0/preview/scim/v2/Me", headers=self.headers)
         # logger.info(res.text)
         if res.status_code == 403:
             logger.error("token not valid.")
             return
         if autoGenerateList:
             logger.info(
-                "autoGenerateList parameter is set to TRUE. Ignoring groupL parameter and instead will automatically generate list of migraiton groups."
+                "autoGenerateList parameter is set to TRUE. "
+                "Ignoring groupL parameter and instead will automatically generate list of migraiton groups."
             )
             self.groupL = self.findMigrationEligibleGroups()
 
@@ -102,7 +104,8 @@ def __init__(
         self.WorkspaceGroupNames = self.groupL
 
         logger.info(
-            f"Successfully initialized GroupMigration class with {len(self.groupL)} workspace-local groups to migrate. Groups to migrate:"
+            f"Successfully initialized GroupMigration class "
+            f"with {len(self.groupL)} workspace-local groups to migrate. Groups to migrate:"
         )
         for i, group in enumerate(self.groupL, start=1):
             logger.info(f"{i}. {group}")
@@ -166,15 +169,17 @@ def findMigrationEligibleGroups(self):
 
             # logger.info count and membership of not_in_account_groups
             logger.info(
-                f"Unable to match {len(not_in_account_groups)} current workspace-local groups. No matching account level group with the same name found. These groups WILL NOT MIGRATE:"
+                f"Unable to match {len(not_in_account_groups)} current workspace-local groups. "
+                f"No matching account level group with the same name found. These groups WILL NOT MIGRATE:"
             )
             for i, group in enumerate(not_in_account_groups, start=1):
                 logger.info(f"{i}. {group} (WON'T MIGRATE)")
 
             if len(migration_eligible) > 0:
                 # logger.info count and membership of intersection
                 logger.info(
-                    f"\nFound {len(migration_eligible)} current workspace-local groups to account level groups. These groups WILL BE MIGRATED."
+                    f"\nFound {len(migration_eligible)} current workspace-local groups to account level groups. "
+                    f"These groups WILL BE MIGRATED."
                 )
                 for i, group in enumerate(migration_eligible, start=1):
                     logger.info(f"{i}. {group} (WILL MIGRATE)")
@@ -183,7 +188,9 @@ def findMigrationEligibleGroups(self):
                 return migration_eligible
             else:
                 logger.info(
-                    "There are no migration eligible groups. All existing workspace-local groups do not exist at the account level.\nNO MIGRATION WILL BE PERFORMED."
+                    "There are no migration eligible groups. "
+                    "All existing workspace-local groups do not exist at the account level."
+                    "\nNO MIGRATION WILL BE PERFORMED."
                 )
                 return []
         except Exception as e:
@@ -250,7 +257,8 @@ def getGroupObjects(self, groupFilterKeeplist) -> list:
                     try:
                         for ent in e["entitlements"]:
                             entms.append(ent["value"])
-                    except:
+                    except Exception as e:
+                        # TBD: introduce warning with proper explanation
                         pass
 
                     groupEntitlements[e["id"]] = entms
@@ -261,7 +269,8 @@ def getGroupObjects(self, groupFilterKeeplist) -> list:
                         try:
                             for ent in e["roles"]:
                                 entms.append(ent["value"])
-                        except:
+                        except Exception:
+                            # TBD: introduce a proper warning
                             continue
                         if len(entms) == 0:
                             continue
@@ -345,7 +354,8 @@ def getRecursiveGroupMember(self, groupM: dict):
             self.groupUserList.extend(userPrincipalList)
             self.groupSPList.extend(spPrincipalList)
 
-    # getACL[n] family of functions extract the ACL from the converted json response into a standard format, filtering by groupL
+    # getACL[n] family of functions extract the ACL
+    # from the converted json response into a standard format, filtering by groupL
     def getACL(self, acls: dict) -> list:
         aclList = []
         for acl in acls:
@@ -386,10 +396,10 @@ def getACL2(self, acls: dict) -> list:
         aclList = []
         for acl in acls:
             try:
-                l = []
+                acls_items = []
                 for k, v in acl.items():
-                    l.append(v)
-                aclList.append(l)
+                    acls_items.append(v)
+                aclList.append(acls_items)
             except KeyError:
                 continue
         for acl in aclList:
@@ -829,7 +839,9 @@ def getExperimentACL(self) -> dict:
                                     f"{self.workspace_url}/api/2.0/permissions/experiments/{expID}",
                                     headers=self.headers,
                                 )
-                    # resExpPerm=requests.get(f"{self.workspace_url}/api/2.0/permissions/experiments/{expID}", headers=self.headers)
+                    # resExpPerm=requests.get(
+                    # f"{self.workspace_url}/api/2.0/permissions/experiments/{expID}", headers=self.headers
+                    # )
                     if resExpPerm.status_code == 404:
                         logger.error("feature not enabled for this tier")
                         continue
@@ -1035,7 +1047,9 @@ def getSingleFolderList(self, path: str, depth: int) -> dict:
             except Exception as e:
                 lastError = e
                 continue
-        logger.error(f"[ERROR] retry limit ({MAX_RETRY}) limit exceeded while retrieving path {path}. last err: {lastError}.")
+        logger.error(
+            f"[ERROR] retry limit ({MAX_RETRY}) limit exceeded while retrieving path {path}. last err: {lastError}."
+        )
         return (path, {}, {}, {})
 
     def getFoldersNotebookACL(self, rootPath="/") -> list:
@@ -1044,7 +1058,8 @@ def getFoldersNotebookACL(self, rootPath="/") -> list:
             # Get folder list
             self.getRecursiveFolderList(rootPath)
 
-            # Collect folder IDs, ignoring suffix /Trash to avoid useless errors. /Repos and /Shared are ignored at the folder list level
+            # Collect folder IDs, ignoring suffix /Trash to avoid useless errors.
+            # /Repos and /Shared are ignored at the folder list level
             folder_ids = [
                 folder_id for folder_id in self.folderList.keys() if not self.folderList[folder_id].endswith("/Trash")
             ]
@@ -1288,7 +1303,9 @@ def getSecretScoppeACL(self) -> dict:
 
                 resSSPermJson = resSSPerm.json()
                 if "items" not in resSSPermJson:
-                    # logger.info(f'ACL for Secret Scope  {scopeName} missing "items" key. Contents:\n{resSSPermJson}\nSkipping...')
+                    # logger.info(
+                    # f'ACL for Secret Scope  {scopeName} missing "items" key. Contents:\n{resSSPermJson}\nSkipping...'
+                    # )
                     # This seems to be expected behaviour if there are no ACLs, silently ignore
                     continue
 
@@ -1487,7 +1504,10 @@ def getDBACL(self, db: str):
                 userList = [p.Principal for p in userListCollect]
                 userList = list(set(userList))
                 if not self.checkPrincipalInGroupOrMember(userList, db):
-                    # logger.info(f'selected groups or members of the groups have no USAGE or OWN permission on database level. Skipping object level permission check for database {db}.')
+                    # logger.info(
+                    # f'selected groups or members of the groups have no USAGE or OWN permission on database level.'
+                    # 'Skipping object level permission check for database {db}.'
+                    # )
                     return []
 
             tables = self.runVerboseSql("show tables in spark_catalog.{}".format(db)).filter(
@@ -1556,7 +1576,8 @@ def getTableACLs(self) -> list:
         userList = list(set(userList))
         if self.checkPrincipalInGroupOrMember(userList, "CATALOG"):
             logger.info(
-                "some groups or members of the group given permission at catalog level, running permission for all databases"
+                "some groups or members of the group given "
+                "permission at catalog level, running permission for all databases"
             )
             self.checkAllDB = True
         database_names = []