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

CLI-520: [pull:files] not enough free disk space #713

Merged
merged 2 commits into from
Nov 4, 2021

Conversation

danepowell
Copy link
Contributor

Motivation
If the files directory doesn't exist, the shell commands don't exit cleanly and the whole process fails.

Proposed changes

  • Ensure files directory exists before trying to read it
  • Capture and rethrow any process errors

Testing steps

  1. Follow the contribution guide to set up your development environment.
  2. Clear the kernel cache to pick up new and changed commands: ./bin/acli ckc
  3. Delete files directory locally, or create a brand new IDE (which will be missing the files directory) and run acli pull.

Merge requirements

  • Bug, enhancement, or breaking change label applied
  • Manual testing by a reviewer

@danepowell danepowell added the bug Something isn't working label Nov 3, 2021
@danepowell
Copy link
Contributor Author

@grasmash in the pull:files test case we mock the localMachineHelper:

protected function mockLocalMachineHelper(): ObjectProphecy {

I've refactored the disk commands into the localMachineHelper as part of this PR, so I don't want to mock those new methods, I want to call the actual method on the localMachineHelper. Is that possible?

@grasmash
Copy link
Contributor

grasmash commented Nov 3, 2021

so I don't want to mock those new methods, I want to call the actual method on the localMachineHelper. Is that possible?

Sure it's possible. You just need to create a real LocalMachineHelper object. That's already done in the setIo() method.

@danepowell
Copy link
Contributor Author

@grasmash the problem is we can't mock just some methods on LocalMachineHelper while calling the original method for others. Mocking is all or nothing. See phpspec/prophecy#332 (comment)

Since this is a rather critical regression, I propose taking the hit in coverage and removing the affected tests for now. We'll follow up immediately to fix them: https://github.com/acquia/cli/issues/714

@danepowell
Copy link
Contributor Author

@grasmash @anavarre please review and test. My sense is that this is critical enough that we should accept the drop in coverage, release a hotfix, and restore coverage in #714

Copy link
Contributor

@anavarre anavarre left a comment

Choose a reason for hiding this comment

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

Manual test worked both for pull and pull:files.

$ /tmp/acli.phar pull
Using Cloud Application MyTestApp

 Choose a Cloud Platform environment [Dev, dev (vcs: pipelines-build-master)]:
  [0] Dev, dev (vcs: pipelines-build-master)
  [1] test, ode28 (vcs: pipelines-build-master)
  [2] ama, ode29 (vcs: pipelines-build-master)
  [3] Prod, prod (vcs: tags/2020-09-28)
  [4] Stage, test (vcs: tags/2020-10-19)
 > 0

    ✔ Pulling code from the Cloud Platform
    ⌛ Copying Drupal's public files from the Cloud Platform...

 ! [NOTE] This is a multisite application. Drupal will load the default site unless you've configured sites.php for this
 !        environment: https://docs.acquia.com/cloud-platform/develop/drupal/multisite/                                 

 Choose a site [myapp]:
  [0] myapp
    ✔ Copying Drupal's public files from the Cloud Platform
        Connecting to database drupal
                                                                                                                        
 [INFO] Using a database backup that is 0 hours old. Backup #190460269 was created at Thu Nov 4 7:12:08 UTC 2021.       
                                                                                                                        
        You can view your backups here:                                                                                 
        https://cloud.acquia.com/a/environments/18672-0e737068-196c-4b7c-8b6e-3fdf0561b428/databases                    
                                                                                                                        
        To generate a new backup, re-run this command with the --on-demand option.                                      
                                                                                                                        

    ✔ Downloading Drupal database copy from the Cloud Platform
    ✔ Installing Composer dependencies
        }
    ✔ Clearing Drupal caches via Drush
    ✔ Sanitizing database via Drush

 Your database was sanitized via drush sql:sanitize. This has changed all user passwords to randomly generated strings. To log in to your Drupal site, use drush uli
$ /tmp/acli.phar pull:files
Using Cloud Application MyTestApp

 Choose a Cloud Platform environment [Dev, dev (vcs: pipelines-build-master)]:
  [0] Dev, dev (vcs: pipelines-build-master)
  [1] test, ode28 (vcs: pipelines-build-master)
  [2] ama, ode29 (vcs: pipelines-build-master)
  [3] Prod, prod (vcs: tags/2020-09-28)
  [4] Stage, test (vcs: tags/2020-10-19)
 > 0

    ⌛ Copying Drupal's public files from the Cloud Platform...

 ! [NOTE] This is a multisite application. Drupal will load the default site unless you've configured sites.php for this
 !        environment: https://docs.acquia.com/cloud-platform/develop/drupal/multisite/                                 

 Choose a site [myapp]:
  [0] myapp
  [1] default
    ✔ Copying Drupal's public files from the Cloud Platform

I agree it's critical enough to accept a slightly decreased test coverage for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants