Refactor Cleanup class #47

ondrajodas · 2025-02-04T22:13:20Z

Optimize code in Cleanup class: Add constants for frequently used strings, Simplify logic in getDataToRemove, Create switchRole method for better readability
Add detailed logging: Info logs for main operations, Debug logs for detailed tracking
Improve type safety: Fix type hints, Proper handling of null values

ondrajodas · 2025-02-06T13:08:23Z

po incidentu kdy nám migrace smazala všechny usery (i ty co to smazat nemělo) jsem to přepsal tak aby to vzalo jen databázovou roli `tu najdu buď od databáze a nebo ze zadaného pole) a našlo to jen usery které ta role vlastní (WS users) + role která ta role vlastní (RO role, WS role)

je to víc přímočarý a jednodušší a nemělo by se tak stt že to smaže něco co nemá...

příklad výstupu:

USE ROLE "SAPI_10346";
REVOKE SELECT ON FUTURE TABLES IN SCHEMA SAPI_10346."in.c-as" FROM ROLE "SAPI_10346_RO";
REVOKE SELECT ON FUTURE TABLES IN SCHEMA SAPI_10346."in.c-keboola-ex-db-snowflake-1223319156" FROM ROLE "SAPI_10346_RO";
REVOKE SELECT ON FUTURE TABLES IN SCHEMA SAPI_10346."in.c-test" FROM ROLE "SAPI_10346_RO";
DROP USER IF EXISTS "SAPI_WORKSPACE_1225392703";
DROP USER IF EXISTS "SAPI_WORKSPACE_1225392706";
DROP USER IF EXISTS "SAPI_WORKSPACE_1225392709";
DROP USER IF EXISTS "SAPI_WORKSPACE_1225392716";
USE ROLE "KEBOOLA_STORAGE";
DROP USER IF EXISTS "SAPI_10346";
USE ROLE "SAPI_10346";
DROP ROLE IF EXISTS "SAPI_10346_RO";
DROP ROLE IF EXISTS "SAPI_WORKSPACE_1225392703";
DROP ROLE IF EXISTS "SAPI_WORKSPACE_1225392706";
DROP ROLE IF EXISTS "SAPI_WORKSPACE_1225392709";
DROP ROLE IF EXISTS "SAPI_WORKSPACE_1225392716";
USE ROLE "KEBOOLA_STORAGE";
DROP ROLE IF EXISTS "SAPI_10346";
DROP DATABASE IF EXISTS "SAPI_10346_OLD";
ALTER DATABASE IF EXISTS "SAPI_10346" RENAME TO "SAPI_10346_OLD";

ondrajodas · 2025-02-06T13:09:51Z

dal jsem na review i TF, se kterým jsem ten tool dělal a byl i u toho incidentu :)

romantmb

Za mě ok, ale úplně se neorientuju, počkal bych ještě na @tomasfejfar

src/Cleanup.php

tomasfejfar

Jako působí to, že to bude fungovat :) Akorát ten naming je teda 🙈

tomasfejfar · 2025-02-06T15:29:30Z

src/Cleanup.php

@@ -33,7 +37,14 @@ public function sourceAccount(): void
            $databaseRole = $this->sourceConnection->getOwnershipRoleOnDatabase($database);
            $projectUser = $this->getProjectUser($databaseRole);

-            $data = $this->getDataToRemove($this->sourceConnection, $databaseRole);
+            $grantedOnDatabaseRole = $this->sourceConnection->fetchAll(sprintf(
+                'SHOW GRANTS ON ROLE %s',


Myslel jsem, že jet o TO ROLE? ale funguje podle dokumentace oboje :D

Tohle teda vezme všechny granty, která má project role

SHOW GRANTS ON ROLE KEBOOLA_123

tohle souvisí i s tím druhým commentem
SHOW GRANT TO ROLE KEBOOLA_123 - načte všechny granty vlastněné touto rolí

SHOW GRANT ON ROLE KEBOOLA_123 - načte granty které vlastní tuto roli - takže KEBOOLA_STORAGE

tomasfejfar · 2025-02-06T15:30:44Z

src/Cleanup.php

@@ -33,7 +37,14 @@ public function sourceAccount(): void
            $databaseRole = $this->sourceConnection->getOwnershipRoleOnDatabase($database);


Owner DB je "project role".

KEBOOLA_123

jo je...jo je to role která vlastní databázi, ano projektová role no

tomasfejfar · 2025-02-06T15:31:31Z

src/Cleanup.php

+                'SHOW GRANTS ON ROLE %s',
+                Helper::quoteIdentifier($databaseRole)
+            ));
+            assert(count($grantedOnDatabaseRole) === 1);


Nrozumím proč je jen jeden ten grant. Může jich být imho klidně víc - třeba na ws usery, schemata, ws role, etc., ne?

Jako je super, že je tu assert, ale nerozumím proč to funguje

funguje to protože - #47 (comment)

src/Cleanup.php

tomasfejfar · 2025-02-07T13:26:57Z

src/Cleanup.php

-                    );
-                    $this->destinationConnection->useRole($role['granted_by']);
-                }
+                $dataToRemove = $this->getDataToRemove($this->destinationConnection, $roleName, $mainRoleName);


A co projektový user KEBOOLA_123?

jestli myslíš smazání ho tak se přidává v metodě "addProjectRoleAndUserToRemove"

tomasfejfar · 2025-02-07T13:27:35Z

src/Cleanup.php

-                $sqls[] = sprintf('USE ROLE %s;', Helper::quoteIdentifier($currentRole));
+            // Drop users owned by roles
+            if (!empty($dataToRemove['USER'])) {
+                $this->logger->info(sprintf('Dropping %d users', count($dataToRemove['USER'])));


Tady bych zalogoval které přesně to dropuje, ne?

to samé - #47 (comment)

tomasfejfar · 2025-02-07T13:27:52Z

src/Cleanup.php

-            $sqls[] = sprintf('DROP ROLE IF EXISTS %s;', Helper::quoteIdentifier($databaseRole));
+            // Drop roles
+            if (!empty($dataToRemove['ROLE'])) {
+                $this->logger->info(sprintf('Dropping %d roles', count($dataToRemove['ROLE'])));


Taky bych si myslel, že by tu mělo být které přesně

no nevím jestli bych to tu měl vypsat 🤷‍♂️ seznam který chceme migrovat těch rolí a userů má několik stovek/tisíc

tomasfejfar · 2025-02-07T13:29:21Z

src/Cleanup.php

@@ -297,36 +306,100 @@ public function postMigration(): void
        }
    }

-    private function getDataToRemove(Connection $connection, string $role): array
+    private function getDataToRemove(Connection $connection, string $name, string $mainRoleName): array


tyjo, ten naming... $roleName, ne?

a getDataToRemoveForRole

naming zajišťoval cursor, tak jsem se svezl... přejmenuju

src/Cleanup.php

tomasfejfar

Asi LGTM :)

ondrajodas · 2025-02-10T21:59:07Z

@tomasfejfar sry ještě jednou.. on je tu dismiss když pošlu commit 🤦

ondrajodas added 4 commits February 4, 2025 23:09

Refactor Cleanup class: optimize code, add logging, improve type safety

3300a40

Add lowercase role name support

9d1399d

Add mainRoleName retrieval from database grants

0b49458

Simplify getDataToRemove method and improve code readability

93c064a

ondrajodas requested review from romantmb and tomasfejfar February 6, 2025 13:09

romantmb previously approved these changes Feb 6, 2025

View reviewed changes

src/Cleanup.php Outdated Show resolved Hide resolved

tomasfejfar previously approved these changes Feb 7, 2025

View reviewed changes

changes after review

3af4bb3

ondrajodas dismissed stale reviews from tomasfejfar and romantmb via 3af4bb3 February 10, 2025 21:47

rename getDataToRemove to getDataToRemoveForRole

2f2cc3b

ondrajodas requested a review from tomasfejfar February 10, 2025 21:49

tomasfejfar previously approved these changes Feb 10, 2025

View reviewed changes

fix phpcs

6cdf4d6

ondrajodas dismissed tomasfejfar’s stale review via 6cdf4d6 February 10, 2025 21:55

ondrajodas requested a review from tomasfejfar February 10, 2025 21:59

tomasfejfar approved these changes Feb 10, 2025

View reviewed changes

ondrajodas merged commit ed21240 into main Feb 13, 2025
4 checks passed

ondrajodas deleted the premigration-cleanup-refactoring branch February 13, 2025 20:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Cleanup class #47

Refactor Cleanup class #47

ondrajodas commented Feb 4, 2025 •

edited

Loading

ondrajodas commented Feb 6, 2025

ondrajodas commented Feb 6, 2025

romantmb left a comment

tomasfejfar left a comment

tomasfejfar Feb 6, 2025

tomasfejfar Feb 6, 2025

tomasfejfar Feb 7, 2025

ondrajodas Feb 10, 2025

tomasfejfar Feb 6, 2025

tomasfejfar Feb 7, 2025

ondrajodas Feb 10, 2025

tomasfejfar Feb 6, 2025

tomasfejfar Feb 6, 2025

ondrajodas Feb 10, 2025

tomasfejfar Feb 7, 2025

ondrajodas Feb 10, 2025

tomasfejfar Feb 7, 2025

ondrajodas Feb 10, 2025

tomasfejfar Feb 7, 2025

ondrajodas Feb 10, 2025

tomasfejfar Feb 7, 2025

tomasfejfar Feb 7, 2025

ondrajodas Feb 10, 2025

tomasfejfar left a comment

ondrajodas commented Feb 10, 2025

		@@ -33,7 +37,14 @@ public function sourceAccount(): void
		$databaseRole = $this->sourceConnection->getOwnershipRoleOnDatabase($database);

Refactor Cleanup class #47

Refactor Cleanup class #47

Conversation

ondrajodas commented Feb 4, 2025 • edited Loading

ondrajodas commented Feb 6, 2025

ondrajodas commented Feb 6, 2025

romantmb left a comment

Choose a reason for hiding this comment

tomasfejfar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomasfejfar left a comment

Choose a reason for hiding this comment

ondrajodas commented Feb 10, 2025

ondrajodas commented Feb 4, 2025 •

edited

Loading