Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

!!! FEATURE: High level Neos site import site:importAll #5307

Merged
merged 59 commits into from
Nov 12, 2024

Conversation

bwaidelich
Copy link
Member

@bwaidelich bwaidelich commented Oct 21, 2024

Add commands site:importall, site:exportall and site:pruneall to import export and prune all sites of the given content repository. This will export events files and sites into a package (Resources/Private/Content) or any other specified target directory.

Further changes:

  • Remove contentStreamId and workspaceName from exported events as those are overwritten on import anyways
  • Remove commands cr:import, cr:export and cr:prune because those did not take the site nodes into account.

Todos

  • More inline docs
  • site:export command
  • Add sites.json to export
  • Add "check for empty content stream" processor (for better error handling)
  • Extract workspace creation from events processor (only create if exists)
  • Remove workspaceName and contentStreamId from events.jsonl
  • Re-add lowlevel import/export service (to make review easier) I re-added the tests and test-tools for now, I would like to skip the services and cli commands for now (until the rest is "safe")
  • Make LegacyMigrationService use high-level import
  • \Neos\ContentRepository\LegacyNodeMigration\NodeDataToEventsProcessor (now \Neos\ContentRepository\LegacyNodeMigration\Processors\EventExportProcessor) has to work with workspaceName instead of contentStreamId -> solved

Resolves: #4448

@github-actions github-actions bot added the 9.0 label Oct 21, 2024
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I decided to remove those low-level services for now.
It is quite easy to create them if needed (and more configurable then) but for Neos we'll need something more "highlevel" anyways

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know, "context" is not the best pattern, but in this case it really makes sense IMO:
It's a common object that is passed through all processors allowing them a shared access to the (virtual) filesystem and to dispatch "events"

(needs doc comments)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a plain (for now) collection object for processors that allows us to pass them around

@mficzel
Copy link
Member

mficzel commented Oct 21, 2024

In #5306 we just moved cr:prune to the CrCommandController into the Neos.Neos->CrCommandController Package that gets deleted here.

Copy link
Member Author

@bwaidelich bwaidelich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking great already, thanks for taking over. Just a few remarks

…d `site:exportAll` to be in line with current behavior
- centralize pathDetermination and closure generationIn site command controller
- use catch-up instead of replayall after import
- remove workspace name from exported events
@mficzel
Copy link
Member

mficzel commented Oct 23, 2024

Wonder about "Make LegacyMigrationService use high-level import" ... this would make the legacy migration depend on Neos.Neos ... technically there is no non Neos way to get an old Neos CR but it seems not that elegant anyways.

…te ProjectionCatchUpProcessor and ProjectionReplayProcessor.
…tLegacyData` alongside `cr:migrateLegacyData`
@bwaidelich bwaidelich changed the title WIP: FEATURE: Site import FEATURE: Site import Oct 24, 2024
Copy link
Member

@mhsdesign mhsdesign left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Giving this +.5 already ;) will need to test that again with the Neos ui e2e tests (as they use the export as well) but for now it looks really good. We also have to adjust the flow setup to guide to these new commands :)

As written here #5307 (comment) im really questioning if migrateLegacyData should be a one step. We alreay require that people migrate to the Neos.Neos:Site homepage nodetype and their dimension config either way.

One more - good imo - step in between would just allow more flexibility and "come back later" to work on the migration.

…w_identity_neos_neos_domain_model_domain`

Instead, we will now gracefully handle this case:

Domain "onedimension.localhost" already exists. Adding it to site "Neos Test Site".
@mhsdesign
Copy link
Member

I'd be fine with that. As long as there is no cryptic exception when forgetting to execute a number of commands in a very specific order ;)

this would be it but could further be improved ... but idk ... :D Its just the flow command line exception handling printer :)

image

also im just testing this and it works so far, i guess its okay that the export overrides the directory specified but could also be dangerous?
maybe:

$directoryContainsFiles = (new \FilesystemIterator($path))->valid();
if ($directoryContainsFiles) {
    $this->outputLine(sprintf('Path %s is not empty.', $path));
    $this->quit(1);
}

In our case we want to skip the creation of a domain if it exists based on the host name (see c1d64ba) but now we always skip sub domains, if the main domain is created first.
…ta`)

replaced with

```
./flow site:exportLegacyDataCommand --path ./migratedContent
./flow site:importAll --path ./migratedContent
```
previously `primary` was always true if there is only one domain
previously we didnt use the value that was in the export format.
Copy link
Member

@mhsdesign mhsdesign left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay i spend a lotta time testing this extensively now and fixing bugs and edgecases.

I tested doing a exportLegacyData with my local Neos 8 demo + domain and another site i use for Neos UI tests.
Then i imported that one again into Neos 9 testet the result via ui.
And to make it a full circle i exported that result again via exportAll and diffed the folders as we expect the same result - which it now finally is.

Following fixed bugs:

  • imported sites are not active
  • primary domain export was not correct
  • having the same domain already was not a noop but crashed due to constraints
  • fail early if --path is not a directory
  • respect copyrightNotice of serialised assets during import

Now im at the state to say we can merge :D

@mhsdesign mhsdesign changed the title FEATURE: Site import !!! FEATURE: Site import Nov 10, 2024
@bwaidelich
Copy link
Member Author

Thanks a lot for putting so much effort into this one, everybody!
Let's get this beast merged at some point. We can always tweak it later

@mhsdesign
Copy link
Member

As per martin from slack

Fein

:D lets merge!

@mhsdesign mhsdesign changed the title !!! FEATURE: Site import !!! FEATURE: High level Neos site import site:importAll Nov 12, 2024
@mhsdesign mhsdesign merged commit 3a5b6c5 into 9.0 Nov 12, 2024
12 checks passed
@mhsdesign mhsdesign deleted the feature/4448-site-import branch November 12, 2024 09:24
@kitsunet
Copy link
Member

kitsunet commented Nov 17, 2024

UI tests are now updated.

@mhsdesign
Copy link
Member

Lol i just found an early draft in our git history eaf246c the export thing was now reworked also three times 😅 :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FEATURE: High level cr:import / Read site:import
5 participants