Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
For the Aegir project it would be usefull to create archives of just the site specifics.
Therefore --no-core was opted in #1138882: Support drush archive-dump
I've started some work on this in a sandbox, dump seems to be working.
I've added an extra testCase for this functionality,
See: http://drupalcode.org/sandbox/helmo/1277350.git
The current method of restore is an issue though as drush_move_dir() is used which does not like an existing target directory. This use-case would need a 'copy' method.
The sandbox also contains a few commits with general cleanup.
Comment | File | Size | Author |
---|---|---|---|
#39 | drush-no_core_option_for_archive_dump-7.x-5.x-1277484-39.patch | 13.87 KB | vinmassaro |
#39 | drush-1277484-844224-combined-39.patch | 17.73 KB | vinmassaro |
#33 | drush-1277484-interdiff.patch | 15.49 KB | jonhattan |
#33 | drush-1277484.patch | 13.41 KB | jonhattan |
#32 | drush-no_core_option_for_archive_dump-1277484-32.patch | 12.53 KB | vinmassaro |
Comments
Comment #1
moshe weitzman CreditAttribution: moshe weitzman commentedLooks good. I think you can just run with --no-core in the existing test and check that all looks good. I'm OK with creating a new test, but I fail to see why we need to clear the sandbox between tests. Can't we just generate a second archive into a different --destination?
Lets discuss restore in a separate issue.
Comment #2
helmo CreditAttribution: helmo commentedIncluding this test in testArchiveDump() would make it longer then I would prefer for a method.
I also have the desire to extend this even further to be able to archive just a platform, without any sites. Then it's just easier to add a third case then to again lengthen this one.
The reason i chose to clear the sandbox was that it's not only the dump filename that conflicts but also the untar directory and the $expected string.
To get the cleanest possible test, while preserving some cached data, this seemed like the best idea,
I'd suggest calling a new method e.g. setUpFreshSandBox() instead of setUpBeforeClass().
setUpBeforeClass() would then delegate all it's work to this new method.
Comment #3
moshe weitzman CreditAttribution: moshe weitzman commentedIt sounds like archive-dump is not cleaning itself up properly after making an archive? I don't see why the sandbox should be polluted with anything but a single tarball.
Anyway, refactoring into setUpFreshSandBox() sounds good. Feel free to put it into this patch.
Comment #4
helmo CreditAttribution: helmo commentedI pushed the refactorring of setUpFreshSandBox() into my sandbox.
The 'untar' directory is a result from test code, so not a problem of archive-dump itself.
Comment #5
moshe weitzman CreditAttribution: moshe weitzman commentedWell, lets clean up after ourselves in the unit test code then. We can keep the refactoring but I don't want to encourage a pattern where methods are clearing sandbox 'just in case'. Lets not actually call the new method.
Comment #6
helmo CreditAttribution: helmo commentedok, it's now cleaning up in the test itself.
I've also opened an issue for the archive-restore part: #1280218: archive-restore adaption for an archive made with --no-core option
Comment #7
moshe weitzman CreditAttribution: moshe weitzman commentedYou can't call a drush function directly from a functional test. You can do it from a pure unit test since those do bootstrap drush in same process. I'm surprised (and worried) that you are not seeing a fatal when running just this test.
Instead call $this->drush('eval', ' drush_delete_tmp_dir()l')
Comment #8
helmo CreditAttribution: helmo commentedEval... :( no.
Gladly I found unish_file_delete_recursive() as a replacement function.
Comment #9
anarcat CreditAttribution: anarcat commented@moshe - i thought we could call functions directly in unit tests now that the unit testing framework was rewired?
Comment #10
moshe weitzman CreditAttribution: moshe weitzman commentedPlease post a patch instead of (or in addition to) a link to your sandbox. i would like to review the diff but not having it easily available is a deterrent.
@anarcat - sure, thats right. IIRC, the patch here did not use a unit test but rather a traditional system test.
Comment #11
moshe weitzman CreditAttribution: moshe weitzman commentedI'd like to get this committed for Aegir.
Comment #12
moshe weitzman CreditAttribution: moshe weitzman commentedComment #13
helmo CreditAttribution: helmo commentedHere's the patch from a diff of origin/master..sandbox/master-archive-dump
Comment #14
helmo CreditAttribution: helmo commentedI recently discovered a way to link to a diff from drupalcode.org.
This link shows all changes in my branch: http://drupalcode.org/sandbox/helmo/1277350.git/commitdiff/master-upstre...
And as plain text: http://drupalcode.org/sandbox/helmo/1277350.git/commitdiff_plain/master-...
I cleaned up some whitespace errors in the meantime....
Comment #15
mortona2k CreditAttribution: mortona2k commentedI would really like to use --no-core, and I am having trouble applying the patch. I am using drush 5 rc 3.
EDIT: I just copied your archive.drush.inc file from git, and everything seems to be working smoothly. Thanks!
Comment #16
moshe weitzman CreditAttribution: moshe weitzman commentedThat diff is now huge and contains unrelated changes
Comment #17
helmo CreditAttribution: helmo commentedNot really surprising that the patch no longer applies...
Is there a realistic chance of this getting in anytime soon? If so then I'll see if I can re-roll.
Comment #18
helmo CreditAttribution: helmo commentedAfter all this time... I've rebased.
I've split off a some cleanup changes that are beneficial to this patch(also in my sandbox)
I can make a separate issue for it if you really like, but they should be easy to review/accept.
Please review to get this finally in(sandbox branch).
Tests from archiveDumpTest.php still run OK after this.
Comment #19
helmo CreditAttribution: helmo commentedI created a separate issue for the cleanup patch: #1616416: Cleanup patch
Comment #20
helmo CreditAttribution: helmo commentedAs the cleanup patch has been committed to master, I've updated the rest with one comment from @moshe weitzman
Comment #21
vinmassaro CreditAttribution: vinmassaro commentedWould like to see this as well but the patch in #20 did not apply cleanly for me.
Comment #22
helmo CreditAttribution: helmo commentedOK, I've re-rolled the patch
Comment #23
vinmassaro CreditAttribution: vinmassaro commentedPatch applies cleanly and works great for me. Produces a database and the sites/default directory.
Comment #24
vinmassaro CreditAttribution: vinmassaro commentedComment #25
mortona2k CreditAttribution: mortona2k commentedApplies cleanly to 5.6 as well.
Comment #26
vinmassaro CreditAttribution: vinmassaro commentedWill this make it into 7.x-5.8?
Comment #27
vinmassaro CreditAttribution: vinmassaro commented@helmo, can you reroll the patch against 7.x-5.x HEAD? Mentioned this issue on IRC in #drush, hoping to get maintainer review so this can be committed. Thanks!
Comment #28
helmo CreditAttribution: helmo commentedI'd love to (again) but the lack of progress is very discouraging...
The related issue #1280218: archive-restore adaption for an archive made with --no-core option has gone over a year without any review :(
Comment #29
helmo CreditAttribution: helmo commented@vinmassaro: Could you maybe help in http://groups.drupal.org/site-archive-format to get a page up with a definitive format description?
I've tried to in #1294632: Document Site Archive Format (my draft)
Comment #30
vinmassaro CreditAttribution: vinmassaro commented@helmo, not sure how I can hep in #29 - your draft is looking pretty good.
I followed the patch re-roll guide and was able to get it updated against HEAD (correctly, I hope).
Comment #31
jonhattanPatches must be against 6.x-dev branch. Anyway here's a preliminary review:
Let's be more precise in the description.
I guess this option just add sites/default or sites/example.com to the tarball.. Isn't it?
Other fields in the manifest are:
formatversion, generator, generatorversion
. In concordance the new field should bearchiveformat
.Also platform is an Aegir word. I'm fine with it but perhaps there's a better term.
That whitespace in the error code is not introduced by this patch, but just seen it now. It should be DRUSH_ARCHIVE_UNABLE_TO_RESTORE_FILES. Feel free to fix it, or I'll fix it once this issue lands.
A more consistent way: drush_drupal_version($docroot)
Comment #32
vinmassaro CreditAttribution: vinmassaro commented@jonhattan, rerolled against 8.x-6.x and includes your suggested changes from #31 except for changing the term 'platform'. Wasn't sure how to add credit for helmo since he created the original patch. Thanks!
Comment #33
jonhattanI've reworked some parts of the patchin #32, including my own suggestions in #31 and a major refactor of the tests. I'm attaching the interdiff and the full patch for reference.
Comment #34
jonhattanCommitted to 6.x.
Comment #35
vinmassaro CreditAttribution: vinmassaro commentedThis is great, thanks! How does this make it into 7.x-5.x for the next Drush release?
Comment #36
greg.1.anderson CreditAttribution: greg.1.anderson commentedSomeone would need to backport this to the 5.x branch. Setting status for consideration.
Comment #37
jonhattanI'm for backport, but the patch doesn't apply because of the commits done within #1844224: Document how to correctly format filesystem paths for Windows shells and insure existing usage is consistent and I'm not sure how to proceed.
Comment #38
greg.1.anderson CreditAttribution: greg.1.anderson commentedI think that #844224 could be backported to 5.x as well.
Comment #39
vinmassaro CreditAttribution: vinmassaro commentedI re-rolled #844224 against 7.x-5.x and posted it in that issue, and here is a re-roll of this patch against 7.x-5.x, as well as a combined patch from both issues. I believe I did this correctly, but if this is a mess, please let me know!
Comment #40
jonhattanbackported.
@vinmasaro thanks for the patchs. That's not neccesary since the 6.x version can be applied to 5.x once #1844224: Document how to correctly format filesystem paths for Windows shells and insure existing usage is consistent was backported.
Comment #41
vinmassaro CreditAttribution: vinmassaro commentedJust wanted to follow-up with how this is helping us at Yale. We allow site developers to fork our main d7 repo, and work out of their fork. Commits to their forked repo trigger a Jenkins build that deploys the updated code to their site. At the beginning of each build, drush archive-dump runs to create a backup. This can potentially add a substantial amount of time to the build, depending on the size of the database and files, as well as git data.
Using --no-core, we are able to drop a lot of time from builds, and this is especially helpful if developers are making small iterative commits.
On a site with ~100mb of files in the tree (including git data) and ~50mb uploaded files running on MAMP as a test, we see an ~88% decrease in archive-dump time:
As well as a ~71% decrease in disk space used:
Comment #42
helmo CreditAttribution: helmo commentedThanks all for getting this committed.