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

Improve archive extraction path error handling #1054

Merged
merged 3 commits into from
Aug 15, 2018

Conversation

andrewfrench
Copy link
Contributor

The Problem/Issue/Bug:

The review in #994 describes some issues with import-files, namely one where a bad extraction path would return a success despite having extracted zero files into the project.

Additionally, a regression was identified where a user would no longer be prompted for an archive extraction path when using the import-files interactive mode.

How this PR Solves The Problem:

If no files match the extraction path in an archive, an error is returned to the user.

The interactive extraction path regression was fixed as well.

Manual Testing Instructions:

Provide an archive in interactive mode and ensure the extraction path prompt is presented.

Provide an invalid extraction dir and ensure an error is generated.

Automated Testing Overview:

Related Issue Link(s):

#944

@andrewfrench andrewfrench requested a review from rfay August 15, 2018 17:41
@andrewfrench andrewfrench self-assigned this Aug 15, 2018
@andrewfrench andrewfrench added this to the v1.1.0 milestone Aug 15, 2018
Copy link
Contributor

@rickmanelius rickmanelius left a comment

Choose a reason for hiding this comment

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

I still note a regression. Here's my testing pattern:

  • Interact Mode
    • PASS: Incorrect file: Errors out correctly
    • PASS: No extract path: Imports entire archive correctly
    • PASS: Correct file and extract path: Imports correctly
  • Flag Mode
    • PASS: Incorrect file: Errors out correctly
    • FAIL: No extract path: Claims imported but it doesn't.
    • PASS: Correct file and extract path: Imports correctly

To note, here was the command I used to get the false positive: ddev import-files --src db.20180815_104847.tar.gz

@rickmanelius rickmanelius self-requested a review August 15, 2018 19:15
Copy link
Contributor

@rickmanelius rickmanelius left a comment

Choose a reason for hiding this comment

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

After a screen share and retest with @andrewfrench, it appears that it was a testing error on my side. I must have been validating against a different directory with an empty repo for that one test. We tested each scenario again and we passed.

@rfay
Copy link
Member

rfay commented Aug 15, 2018

Ooops, I get No upload directory has been specified for the project type drupal6 on a d6 site - that's doing it from just ddev import-files with no flags

@rfay
Copy link
Member

rfay commented Aug 15, 2018

Cancel that. I had wrong ddev. quicksprint installed a new one for me!

Copy link
Member

@rfay rfay left a comment

Choose a reason for hiding this comment

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

A modest amount of testing and scan through the code ran into no problems although I learned about the difference between archives with ./

structure and straight structure. I think ./ is more likely; we'll deal with this in the future if it comes up.

@andrewfrench andrewfrench merged commit f9acd24 into ddev:master Aug 15, 2018
@andrewfrench andrewfrench deleted the 2018-08-15_import-files-fix branch August 15, 2018 20:15
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.

None yet

3 participants