neos / neos-development-collection

The unified repository containing the Neos core packages, used for Neos development.
https://www.neos.io/
GNU General Public License v3.0
260 stars 221 forks source link

TASK: Remove `--reset-projections` flag from `cr:setup` command #5089

Closed bwaidelich closed 3 months ago

bwaidelich commented 4 months ago

Removes the --reset-projections option from the cr:setup command because that could fail

Fixes: #5008

mhsdesign commented 4 months ago

Thanks for implementing the stuff i complain about but now i understood what you thought and wrote at the other place. I simply ruled out this as this will defeat the purpose of --reset-projections. The flag is meant as hardcore fix in case we introduce a new schema config that cannot! be setup'd without having to empty the projections (because the content cant be migrated) see christians integer change etc.

Now the flag works well for this case, but if also a new table is introduced the reset will complain due to the table not existing. Its a henn egg problem. Cant do any of them first. Thus i wanted to do a forgiving reset - which should be imo fine.

bwaidelich commented 4 months ago

@mhsdesign can you elaborate in which cases a setUp() would break? IMO it should never. What is "christians integer change"?

kitsunet commented 4 months ago

The relation anchors, that required an empty DB or no tables to exist as the existing table was incompatible with the new schema.

bwaidelich commented 4 months ago

The relation anchors, that required an empty DB or no tables to exist as the existing table was incompatible with the new schema.

Ah, right. But I stick to my claim: IMO setUp() should never fail. If the SchemaDiff can't handle it automatically we can add some custom statements to make it stable.

In general: I want to be able to run ./flow cr:setup in my CI

mhsdesign commented 4 months ago

sure for ci it must always work when were 9.0 stable. but currently --reset-projections was a nice bruteforce introduced with https://github.com/neos/neos-development-collection/pull/4864

We can remove this flag again as well but we thought at that time it was a good idea, or have we all forgotten :D

bwaidelich commented 4 months ago

I'm OK with the flag and I'm confident that we can keep setUp non-crashing from now on

mhsdesign commented 4 months ago

TLTR; remove it plz

I'm confident that we can keep setUp

haha no xD flow cr:setup --reset-projections will fail again soon. Once we introduce a new table for the content graph projection again with https://github.com/neos/neos-development-collection/pull/5096

but that shows surely that it was a bad idea from me to introduce it ^^

though having our beta users have to run

flow cr:resetall --content-repository default // ignore the error it yields
flow cr:setup --content-repository default
flow cr:replayall --content-repository default

was also not cool thats why we introduced it. But it can go now idk. We dont plan any changes like https://github.com/neos/neos-development-collection/pull/4791 anymore

bwaidelich commented 4 months ago

flow cr:setup --reset-projections will fail again soon. Once we introduce a new table for the content graph projection again

Why should that fail?? setUp() will just create the new table, that's what it's there for – Or what am I missing?

Anyways, I'm fine with removing the flag – usually you want to trigger a catchup after reset anyways

mhsdesign commented 4 months ago

hen egg situation as said. One must basically now if this flag should be used now or not.

setUp() will just create the new table, that's what it's there for – Or what am I missing?

in this case a reset before a setup will fail.

But changing the order will not fix the original problem this flag tried to solve see https://github.com/neos/neos-development-collection/pull/4791#issuecomment-1910177290.

But as we dont want any breaking changes anymore in the schema that cant be automigrated with filled data (asking @kitsunet here?) we can drop it

mhsdesign commented 4 months ago

cant we just share one singleton instance of my brain part that holds the cr parts :D