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

feat(import): add --allow-system-documents to dataset import #5689

Merged
merged 1 commit into from
Feb 12, 2024

Conversation

j33ty
Copy link
Contributor

@j33ty j33ty commented Feb 7, 2024

Description

As part of the import process through CLI and @sanity/import package, we prefer that users do not send in system documents. These documents are considering to be an internal detail and if imported, their interaction with existing system documents is undefined.

This PR attempts at adding a new flag --allow-system-documents to sanity dataset import CLI and an option allowSystemDocument to @sanity/import package, to explicitly permit system document import if the user knows what they are doing.

This can be considered to be a breaking change since it by default skips system documents from import, but I highly doubt anyone is importing system documents from CLI. And, if they are, they should stop doing that. Ideally, I should add data to my gut-feeling and figure out how many users are importing system documents directly from CLI but it's not trivial to get these numbers. Let me know if this needs to be done.

Background for change is present in #prj-dataset-backups.

Bikeshedding

Two appropriate names for the flag are --import-system-documents or --allow-system-documents. I have weak preference for the latter since it aligns with existing options on the same subcommand.

Testing

Two unit tests are added where import with and without this flag is tested.

Notes for Release

It should okay to add commit message as it is to release notes.

Copy link

vercel bot commented Feb 7, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
performance-studio ✅ Ready (Inspect) Visit Preview Feb 8, 2024 7:04pm
test-studio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 8, 2024 7:04pm
1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
studio-workshop ⬜️ Ignored (Inspect) Visit Preview Feb 8, 2024 7:04pm

Copy link
Contributor

github-actions bot commented Feb 7, 2024

No changes to documentation

Copy link
Contributor

github-actions bot commented Feb 7, 2024

Component Testing Report Updated Feb 8, 2024 7:07 PM (UTC)

File Status Duration Passed Skipped Failed
comments/CommentInput.spec.tsx ✅ Passed (Inspect) 37s 15 0 0
formBuilder/ArrayInput.spec.tsx ✅ Passed (Inspect) 6s 3 0 0
formBuilder/inputs/PortableText/Annotations.spec.tsx ✅ Passed (Inspect) 13s 3 0 0
formBuilder/inputs/PortableText/copyPaste/CopyPaste.spec.tsx ✅ Passed (Inspect) 13s 4 2 0
formBuilder/inputs/PortableText/Decorators.spec.tsx ✅ Passed (Inspect) 13s 6 0 0
formBuilder/inputs/PortableText/FocusTracking.spec.tsx ✅ Passed (Inspect) 33s 15 0 0
formBuilder/inputs/PortableText/Input.spec.tsx ✅ Passed (Inspect) 18s 9 0 0
formBuilder/inputs/PortableText/ObjectBlock.spec.tsx ❌ Failed (Inspect) 1m 6s 17 0 1
formBuilder/inputs/PortableText/Styles.spec.tsx ✅ Passed (Inspect) 14s 6 0 0
formBuilder/inputs/PortableText/Toolbar.spec.tsx ✅ Passed (Inspect) 20s 9 0 0

… to explicitly permit system document import, which should be ignore otherwise
@j33ty j33ty force-pushed the radhe/cldx-1593-include-system-docs branch from dc96361 to 3e12256 Compare February 8, 2024 18:58
@j33ty j33ty marked this pull request as ready for review February 8, 2024 18:59
@j33ty j33ty requested a review from a team as a code owner February 8, 2024 18:59
@j33ty j33ty requested review from bjoerge, rneatherway and codebymatt and removed request for a team February 8, 2024 18:59
@j33ty j33ty self-assigned this Feb 8, 2024
@j33ty j33ty changed the title CLI: Add --import-system-document to dataset import feat(import): add --allow-system-documents to dataset import Feb 8, 2024
Copy link
Contributor

@rneatherway rneatherway left a comment

Choose a reason for hiding this comment

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

I'd probably also have been happy with just filtering them out and not offering an option to allow it at all.

@j33ty
Copy link
Contributor Author

j33ty commented Feb 9, 2024

I'd probably also have been happy with just filtering them out and not offering an option to allow it at all.

IMO taking away a functionality without a fallback option isn't a good experience. Enterprise folks with custom group documents may want to import system documents and with a flag, they can continue importing them.

@j33ty j33ty requested a review from binoy14 February 12, 2024 17:13
Copy link
Contributor

@binoy14 binoy14 left a comment

Choose a reason for hiding this comment

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

makes sense to me, thanks!

@j33ty j33ty added this pull request to the merge queue Feb 12, 2024
Merged via the queue into next with commit fa9f924 Feb 12, 2024
40 checks passed
@j33ty j33ty deleted the radhe/cldx-1593-include-system-docs branch February 12, 2024 20:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants