Refactor staging provider #315

odinuv · 2024-09-04T11:20:23Z

part of https://keboola.atlassian.net/browse/KAB-257

Hlavní změna spočívá v tom, že jsem zrušil celou abstrakci pro Staging* a celou abstrakci pro WorkspaceProviderFactory*. Byla to vlastně paralellní abstrakce k imlementacím pro ProviderInterface z IM a to je ten důležitý interface, který se používá v IM a OM. Čili strurčně rozdíl je v tom, že se v docker bundle keboola/docker-bundle#762 místo vytváření ProviderFactory se rovnou vytváří Provider.

Veškerej backend dependent kód je naskládanej do
https://github.com/keboola/platform-libraries/pull/315/files#diff-f7574b43c853c63d37d45e07da98047ea7ce019d2e1561e13587ae2001905f7c a
https://github.com/keboola/platform-libraries/pull/315/files#diff-c272cbe4907dbeec8cdd186b9d7d7b9b8ea284ff6daa87a2cad20a7dde81a976
což sice není tak supr objektově čistý jako předtím, ale zase se v tom dá vyznat (podle mě).

Bylo tam 5 factory class, ale finálně vlastně docker bundle potřebuje dvě věci - založit nový workspace (NewWorkspaceStagingProvider), nebo použít existujíci (ExistingWorkspaceStagingProvider).
Pořád platí, že ty WS jsou lazy initialized a cached (i když v rámci běhu docker bundle se teď inicializují dřív.) - nicméně, a to je podstatný provider teď sám o sobě ví, že je nebo není initialized. Díky tomu se dá sem https://github.com/keboola/platform-libraries/pull/315/files#diff-dcfa96334114061794eb7ba0c77f238f9c08aa326e754f85a9b01950712080dcR54 přidat ta podmínka a díky tomu to už nevytváří nový WS jen kvůli tomu aby se udělal cleanup (viz testCleanupDeletedWorkspaceStagingNotInitialized https://github.com/keboola/platform-libraries/pull/315/files#diff-b62a34c0f9af162c555e8ed041dadd7c4d440eb604019b6c941738ff98b58f78R235).
V ExistingWorkspaceStagingProvider https://github.com/keboola/platform-libraries/pull/315/files#diff-d179f2690fe4e7e6d6b3f5a0a5007dff9a1fddd141ea2f816d6b2e09d0a907b1R34 ta podmínka není, protože to nemá side effect. Tenhle if teda řeší https://keboola.atlassian.net/browse/PST-323

Part of https://keboola.atlassian.net/browse/KAB-257 Depends on keboola/platform-libraries#315

part of https://keboola.atlassian.net/browse/KAB-257 also fixes workspace cleanup so that workspace is not created just to be deleted https://keboola.atlassian.net/browse/PST-323

Part of https://keboola.atlassian.net/browse/KAB-257 Depends on keboola/platform-libraries#315

pepamartinec · 2024-09-16T08:59:49Z

@odinuv Nez se do toho pustim, je to draft, bude se na tom jeste neco dit?

romantmb

Pár drobností, jinak za mě super cleanup. 🙂

libs/staging-provider/src/Provider/StorageApiWorkspace.php

libs/staging-provider/src/AbstractProviderInitializer.php

libs/staging-provider/tests/Provider/ExistingWorkspaceStagingProviderTest.php

odinuv · 2024-09-16T10:18:06Z

Draft to už není :)

pepamartinec

Pekne procisteny.

libs/staging-provider/src/Provider/ExistingWorkspaceStagingProvider.php

pepamartinec · 2024-09-16T09:20:59Z

libs/staging-provider/src/Provider/ExistingWorkspaceStagingProvider.php

+
+    protected function getWorkspace(): StorageApiWorkspace
+    {
+        if (empty($this->workspace)) {


Bylo by dobry zkontrolovat konfiguraci PHPStan, protoze ti to tu dovolilo empty check na non-nullable $this->workspace, coz je mi podezrely ze nefailnulo na always false 🤔

Jinak prosim teda bez empty 🙂

Suggested change

if (empty($this->workspace)) {

if ($this->workspace !== null) {

No ten tvůj check by byl always true, ten můj není always true, protože to je uninitialized typed property

bude to fungovat i s isset, strávíš to? :)

Jo jasne, uninitialized.. to je stav, se kterym se snazim radeji moc neoperovat, pokud to neni nezbytne nutny. Kdyz vis, ze to nemusi byt nastaveny (neinicializuje se to v ramci konstruktoru), osobne bych to cekal jako nullable, no.

Vidíš no a mě to právě přijde jako přesně ten správnej stav. Ten field není nullable - v tom smyslu, že není žádná business logika na základě které by to mělo bejt null

Mně přijde uninitialized jako legitimní stav vlastnosti, pokud s ní nepracuju jinak než s jako lazy-initialized.

Možná bych ten init kód vyndal někam do initWorkspace() func, aby to bylo na první pohled jasný, až bude někdo dělat v budoucnu úpravy.

ale az pres ten getter, tak v klidu pouziju $this->workspace a az kdyz to poprvy zkusim pustit, zkape mi to na unitialized property access erroru. Kdyz je to nullable, je to na prvni pohled videt.

No tak v klidu vidíš, že je to nullable a začneš psát if ($this->workspace === null) ... else ... a po měsíci ti dojde, že to vlastně ve skutečnosti vůbec nullable není :) (vlastní zkušenost)

Ale celkem zajímavej problém no :)

No tak v klidu vidíš, že je to nullable a začneš psát if ($this->workspace === null) ... else ... a po měsíci ti dojde, že to vlastně ve skutečnosti vůbec nullable není :) (vlastní zkušenost)

Nevim co je to za zkusenost, ale prave to, ze zacnes psat if ($this->workspace === null) ... te donuti se podivat proc/jak to vlastne muze byt null, aby si vedel, co s tim delat. Taky vlastni zkusenost 😄

Možná bych ten init kód vyndal někam do initWorkspace() func, aby to bylo na první pohled jasný, až bude někdo dělat v budoucnu úpravy.

Stejny problem, jen jinak pojmenovany. Stejne si musis vsimnout, ze by si mel zavolat initWorkspace nez si na $this->workspace sahnes.

No je to prašť jak uhoď :)

libs/staging-provider/src/Provider/StorageApiWorkspace.php

libs/staging-provider/src/AbstractProviderInitializer.php

libs/staging-provider/src/OutputProviderInitializer.php

libs/staging-provider/src/Provider/Configuration/WorkspaceBackendConfig.php

libs/staging-provider/src/Provider/NewWorkspaceStagingProvider.php

replace empty with isset make AbstractProviderInitializer types more specific

romantmb

👍

Part of https://keboola.atlassian.net/browse/KAB-257 Depends on keboola/platform-libraries#315

pepamartinec

lgtm

pepamartinec · 2024-09-17T08:32:09Z

libs/staging-provider/src/Provider/ExistingWorkspaceStagingProvider.php

+
+    protected function getWorkspace(): StorageApiWorkspace
+    {
+        if (empty($this->workspace)) {


Co me na tom vadi, ze kdyz bych k tyhle class prisel a chtel neco doplnovat, tak pokud si nevsimnu, ze se tam nedela inicializace nejak standardne, ale az pres ten getter, tak v klidu pouziju $this->workspace a az kdyz to poprvy zkusim pustit, zkape mi to na unitialized property access erroru. Kdyz je to nullable, je to na prvni pohled videt.

Dokonce jsem mel za to, ze tohle PHPStan povazuje za antipattern a hlasi. Ale koukam, ze ne 🤔

Anyways, nech to teda asi tak, Roman dal 👍. Ja se zkusim potom potom optat v best-practices, at mame nejaky sirsi nazor.

ErikZigo · 2024-09-17T11:09:19Z

Zaujalo me ze jsem nebyl prizvan k review. No alespon vim ze tu mame vetsi odborniky a muzu se na ne priste obratit

odinuv · 2024-09-17T11:11:34Z

To máš kvůli tomu, žes nebyl v pátek a v pondělí na standupu. No ale jestli k tomu něco máš, tak hurá do toho

ErikZigo · 2024-09-17T12:51:16Z

Z tohodle jsem usoudil, že už bylo řečené vše :)

odinuv · 2024-09-17T18:57:46Z

Já myslím, že je vždycky prostor říct ještě něco

Part of https://keboola.atlassian.net/browse/KAB-257 Depends on keboola/platform-libraries#315

odinuv force-pushed the odin-KAB-257 branch 2 times, most recently from 2ae9b7d to 92c1d4c Compare September 13, 2024 15:43

odinuv changed the title ~~WIP~~ Refactor staging provider Sep 13, 2024

odinuv marked this pull request as draft September 13, 2024 15:43

odinuv mentioned this pull request Sep 14, 2024

Refactor staging-provider keboola/docker-bundle#762

Merged

odinuv added a commit to keboola/docker-bundle that referenced this pull request Sep 14, 2024

Refactor to use refactored staging-provider library

35de48d

Part of https://keboola.atlassian.net/browse/KAB-257 Depends on keboola/platform-libraries#315

odinuv force-pushed the odin-KAB-257 branch from 6e0bfec to 02c4d49 Compare September 14, 2024 21:44

Refactor staging provider

9a258ca

part of https://keboola.atlassian.net/browse/KAB-257 also fixes workspace cleanup so that workspace is not created just to be deleted https://keboola.atlassian.net/browse/PST-323

odinuv force-pushed the odin-KAB-257 branch from 02c4d49 to 9a258ca Compare September 14, 2024 21:46

fix namespaces

ffe4cb5

odinuv added a commit to keboola/docker-bundle that referenced this pull request Sep 15, 2024

Refactor to use refactored staging-provider library

797b7ed

Part of https://keboola.atlassian.net/browse/KAB-257 Depends on keboola/platform-libraries#315

odinuv added a commit to keboola/docker-bundle that referenced this pull request Sep 15, 2024

Refactor to use refactored staging-provider library

741b44d

Part of https://keboola.atlassian.net/browse/KAB-257 Depends on keboola/platform-libraries#315

odinuv mentioned this pull request Sep 16, 2024

Add ability to supply existing workspace not managed by the runner keboola/job-runner#315

Merged

odinuv requested review from pepamartinec and romantmb September 16, 2024 07:11

romantmb requested changes Sep 16, 2024

View reviewed changes

odinuv marked this pull request as ready for review September 16, 2024 10:17

pepamartinec reviewed Sep 16, 2024

View reviewed changes

odinuv added 2 commits September 16, 2024 13:29

add backend to StorageApiWorkspace

512f980

replace empty with isset make AbstractProviderInitializer types more specific

fix type hints

c2d7d3a

odinuv requested a review from romantmb September 16, 2024 12:07

romantmb approved these changes Sep 16, 2024

View reviewed changes

odinuv added a commit to keboola/docker-bundle that referenced this pull request Sep 16, 2024

Refactor to use refactored staging-provider library

5c8f68b

Part of https://keboola.atlassian.net/browse/KAB-257 Depends on keboola/platform-libraries#315

odinuv requested a review from pepamartinec September 17, 2024 07:01

pepamartinec approved these changes Sep 17, 2024

View reviewed changes

odinuv merged commit 4f9c6a7 into main Sep 17, 2024
7 checks passed

odinuv deleted the odin-KAB-257 branch September 17, 2024 18:57

odinuv added a commit to keboola/docker-bundle that referenced this pull request Sep 18, 2024

Refactor to use refactored staging-provider library

dbe3fdd

Part of https://keboola.atlassian.net/browse/KAB-257 Depends on keboola/platform-libraries#315

odinuv mentioned this pull request Sep 26, 2024

add getBackendType method #324

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor staging provider #315

Refactor staging provider #315

odinuv commented Sep 4, 2024 •

edited

Loading

pepamartinec commented Sep 16, 2024

romantmb left a comment

odinuv commented Sep 16, 2024

pepamartinec left a comment

pepamartinec Sep 16, 2024

odinuv Sep 16, 2024

odinuv Sep 16, 2024

pepamartinec Sep 16, 2024

odinuv Sep 16, 2024

romantmb Sep 17, 2024

odinuv Sep 17, 2024

odinuv Sep 17, 2024

pepamartinec Sep 17, 2024

odinuv Sep 17, 2024

romantmb left a comment

pepamartinec left a comment

pepamartinec Sep 17, 2024

ErikZigo commented Sep 17, 2024

odinuv commented Sep 17, 2024

ErikZigo commented Sep 17, 2024

odinuv commented Sep 17, 2024

	if (empty($this->workspace)) {
	if ($this->workspace !== null) {

Refactor staging provider #315

Refactor staging provider #315

Conversation

odinuv commented Sep 4, 2024 • edited Loading

pepamartinec commented Sep 16, 2024

romantmb left a comment

Choose a reason for hiding this comment

odinuv commented Sep 16, 2024

pepamartinec 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

romantmb left a comment

Choose a reason for hiding this comment

pepamartinec left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ErikZigo commented Sep 17, 2024

odinuv commented Sep 17, 2024

ErikZigo commented Sep 17, 2024

odinuv commented Sep 17, 2024

odinuv commented Sep 4, 2024 •

edited

Loading