-
-
Notifications
You must be signed in to change notification settings - Fork 9.2k
[Data transfer] Exclude optional admin types in schema check #16268
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
[Data transfer] Exclude optional admin types in schema check #16268
Conversation
| }; | ||
|
|
||
| // TODO: clean up the type checking, which will require cleaning up the typings in utils/json.ts | ||
| // exclude admin tables that are not transferrable and are optionally available (such as audit logs which are only available in EE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
exclude admin tables that are not transferrable
Just a quick note, but that's only true with Strapi transfer commands though. This won't scale to custom transfer scripts that may want to export their audit logs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can rewrite this to support customizing validation strategies when we open it up to custom transfer scripts
| import type { Diff } from '../../../utils/json'; | ||
| import * as utils from '../../../utils'; | ||
|
|
||
| const OPTIONAL_CONTENT_TYPES = ['audit-log'] as const; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a big fan of packages/data-transfer knowing about the content types name & the business logic behind different uids. But except for adding an unsafe mode in the CLI/engine, I don't have another solution
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see your point, and I think we could define these defaults somewhere outside of data-transfer, but not exactly sure where. I definitely think it needs to have safe defaults that are aware of the business logic though, for example we shouldn't require passing in our exclusions every time we create the engine, it already feels clunky and unsafe the way we're handling the entity/link exclusions with a transform every time rather than letting it be more context aware.
packages/core/data-transfer/src/engine/validation/schemas/index.ts
Outdated
Show resolved
Hide resolved
…x.ts Co-authored-by: Jean-Sébastien Herbaux <jean-sebastien.herbaux@epitech.eu>
|
This pull request has been mentioned on Strapi Community Forum. There might be relevant details there: https://forum.strapi.io/t/data-transfer-from-local-postgresql-to-strapi-cloud/27852/1 |
What does it do?
Excludes admin types that are non-transferrable and can be disabled (eg, by switching between EE or CE) from the schema check on a data transfer. Currently only audit-logs, but should probably include review workflows before the next version
Note: We are currently planning to add a new strategy that only validates the schema for content types that are included in the transfer, and then making that the default for data transfer. So this is a temporary fix that will ensure audit logs and review workflows work, and architectural fixes around how this is working will come later.
Why is it needed?
Currently it is impossible to transfer data between a CE and EE strapi even if the schemas are otherwise the same, because the presence of the audit logs causes a schema validation error.
How to test it?
Use
strapi transfer --toa site that has audit logs enabled from a site that does not have audit logs, but with otherwise identical schemas. Prior to this PR there will be a schema validation check error for audit logs; with this PR the transfer will work.Related issue(s)/PR(s)
Let us know if this is related to any issue/pull request