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

docs: improve discussion of tarball to be imported by ddev import-files, fixes #5474 #5478

Merged
merged 3 commits into from Nov 22, 2023

Conversation

rfay
Copy link
Member

@rfay rfay commented Oct 30, 2023

The Issue

The user there did not understand that the tarball should not contain the top-level directory. I imagine this wasn't the first time.

How This PR Solves The Issue

Update docs and help.

@github-actions
Copy link

github-actions bot commented Oct 30, 2023

@@ -29,6 +29,8 @@ func NewImportFileCmd() *cobra.Command {
The destination directories can be configured in your project's config.yaml
under the upload_dirs key. If no custom upload directory is defined, the app
type's default upload directory will be used.

If importing a Tar or Zip archive, the archive should contain only the *contents* of the top-level target directory. For example, an archive of the Drupal 'sites/default/files' directory should contain only the contents of 'sites/default/files', not the 'files' directory.

Choose a reason for hiding this comment

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

how about adding:

For e.g. In other words, if your archive did contain the top-level "files" directory, then after import the result will be extracted to sites/default/files/files

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 guess IMO that's too much information. It's very specific about how to create the tarball.

Copy link
Collaborator

Choose a reason for hiding this comment

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

aside the detail about content and contents i've asked over in discord.

If using a Tar or Zip archive, the archive should contain only the files inside the files directory. For example in a Drupal site with files at sites/default/files, the archive should *not contain the files directory, just the contents of the files directory

the second sentence took me one or two times until i understood. i wonder if it would make sense to switch the order to what the goal is and then what situation should be avoided. i suppose the wording could still be improved

For example in a Drupal site with files at sites/default/files, the archive should only contain the contents of that `files directory`. You shouldn't end up with a single `files` directory on unarchive containing all the contents.

And i wonder if it would make sense to shorten:

The easiest way to do this with the tar command is to cd into the directory and create the archive there

I would simply strike "the easiest way to do this.." and use something like

To export the files directory use: 

or not sure what matt recommends as the common phrase for pointing at one or two lines of terminal input.

Choose a reason for hiding this comment

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

Agreed, providing an example command will drive the point home. It will take the average person a few reads to completely understand this without an example. Keeping in mind that some automated backup processes do include the parent directory as part of the archive (as was in my case).

$ ddev import-files
Provide the path to the source directory or archive you wish to import.
Please note: if the destination directory exists, it will be replaced with the
import assets specified here.

Choose a reason for hiding this comment

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

adding...

Importantly, all pre-existing files/folders in the target directory will be deleted as a part of this process.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's actually what it already says isn't it?

if the destination directory exists, it will be replaced with the import assets specified here

Choose a reason for hiding this comment

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

Yes you are right. But some people (myself included) would appreciate banging the point home a bit. It is a destructive step after all.

@rfay rfay force-pushed the 20231030_import_files_docs branch from afc7028 to 821141e Compare November 2, 2023 19:59
@rfay
Copy link
Member Author

rfay commented Nov 22, 2023

Ready to go.

@rfay rfay merged commit 0031da1 into ddev:master Nov 22, 2023
16 of 19 checks passed
@rfay rfay deleted the 20231030_import_files_docs branch November 22, 2023 21:32
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.

None yet

3 participants