Allow drush sup to be run multiple times, re-using same code each time

For running site-upgrade, site-upgrade still gives three options:

Drupal site already exists at /home/ben/code/anjali7/web. Would you like to:
[0] : Cancel
[1] : Delete the existing site and start over
[2] : Re-use the existing site, skipping the Drupal download and updatedb
steps

And on picking [2]
Move /home/ben/code/anjali7/web/sites/all/modules to [ok]
/home/ben/code/anjali7/web/sites/all/sites_all_modulesYhD1Pv prior to
updatedb on Drupal core.

What is it doing that i don't get from putting the D6 database right into D7? And is it affecting the database of the site i'm starting from? if so, that should be noted.

Also i'm getting access denied for my user on that database, but i'm sure that's my problem-- i guess it's trying to drop the database?

i'll be back with more useful input soon i hope

but does drush upgrade-site affect its source site? i'm trying to figure out how vigorously i need to clean every database related before trying again.

i can't get an upgrade to run any which way this week. Last week i was able to do so with drush updatedb. All we've done is *disable* modules on the source site, yet that's the one thing that's changed.

So can't for the life of me figure out what's up.

repeatedly getting a table role_permission already exists error.

and last try from the d6 site (with the strange path in the command being where i installed drush master and patched it with this patch) --

/home/ben/code/anjali7/drush/drush site-upgrade @anjali.dev7 --db-su root --db-su-pw toor --no-cache

is hanging, for the past hour or so.

Yeah, sorry about that. Started:
#1074670: What does / should site-upgrade do to the source site? Are any changes made there (and can it be avoided)?
#1074696: Is sql-sync and/or sql-dump leaving out or adding tables or something when passed with no

Apologies, i wasted your time and mine because i somehow dirtied my source site DB, and didn't wipe the tables before pulling it fresh (or so i thought) from production.

Apologies also, i'm testing site-upgrade and not site-install directly.

When running the patched Drush:

drush --debug site-upgrade @anjali.dev7
Immediately after the database drop and import succeeds:

Calling copy(, sites//settings.php)
WD php: copy(): Filename cannot be empty in                          [error]
/home/ben/code/anjali7/drush/includes/drush.inc on line 697. [160.7
sec, 31.54 MB]
Could not create a temporary multisite  [160.7 sec, 31.53 MB]        [error]
Calling is_readable(/home/ben/code/anjali7/web/sites/all/sites_all_modulesDIYJbO)
Calling is_writable(/home/ben/code/anjali7/web/sites/all)
Calling rename(/home/ben/code/anjali7/web/sites/all/sites_all_modulesDIYJbO, /home/ben/code/anjali7/web/sites/all/modules)
Changes made in drush_upgrade_site_upgrade have been rolled back.     [rollback]
[160.71 sec, 31.25 MB]

I don't think it's a permissions issue, the copy command seems to be missing it's variables.

It runs!

Feature request: be able to tell it not to try to download any contrib modules. It checks for every one, asks you to pick which one to download (i haven't tried yet in this version but 0 to cancel at this point canceled the whole script last version), and only then notices it already exists, and asks if you want to override it. Answering n works great at this point but the entire loop of even checking for any of them could be skipped after first time.

Will test this out now. First, some impressions and a code review ...

I'm concerned at how complex this command callback is getting. For example, I don't know that the map in _drush_upgrade_special_projects() is workable. The cck project lives on in D7 just to carry the update functions needed to move to Field API in D7. This pattern is not uncommon. In general, I'm fine with a log msg that states that 'no release available for abssrc'. I think we need to assume that the drush user has good Drupal knowledge.

+++ commands/core/upgrade.drush.inc	23 Feb 2011 14:16:19 -0000
@@ -103,30 +105,37 @@ function drush_upgrade_site_upgrade($tar
+    'replace' => dt("Delete the existing site and start over"),

Sentences should end with period. Happens a few times.

+++ commands/core/upgrade.drush.inc	23 Feb 2011 14:16:19 -0000
@@ -103,30 +105,37 @@ function drush_upgrade_site_upgrade($tar
+    drush_set_option('drupal-version-major', $target_version);

Why do we offer an option about target version. Aren't we always expecting user to upgrade from a working site? The target has to be the next major version, no?

+++ commands/pm/pm.drush.inc	23 Feb 2011 14:16:21 -0000
@@ -222,6 +222,7 @@ function pm_drush_command() {
+      'drush dl drupal --drupal-version-major=6' => 'Download latest recommended release of Drupal 6.x.',

How is different from specifying drupal-6.x? Are we trying to help folks who are rarely work with 7 but frequently work outside of a drupal site? I guess there is some value there.

Hmm. For Drush5, we should consider making dl drupal-6.x download latest recommended and requiring same plus --dev for dev snapshot. that makes the most sense to me.

1. I see now that we are handling the cck upgrade a bit nicer than what I imagined (telling folks that they need to use content_migrate).
2. Would be ideal if sup could run with --simulate and not throw errors.

   I am seeing multiple instances of this error:

   WD php: Undefined index: project in                                  [error]
   /Users/mw/c/h/drush/commands/core/upgrade.drush.inc on line 126.
   

   I saw it happen with the imagefield module, the content module, and date_api module. Not sure why these extensions don't know what their project is.

3. The line drush_pm_download('drupal'); is asking me to confirm 'Install location /Users/mw/htd/sup7 already exists. Do you want to overwrite it? (y/n):'. I've already answered that question earlier (replace).
4. Similarly, perhaps drush_invoke('sql-sync', '@self', $target_key); should run with --yes so we don't ask 'do you want to continue'
5. drush_log(dt("Disable non-core extensions. Could we change to 'Disabling' instead so it is more clear what action we are taking.
6. Am a bit surprised to see:
   

   filefield was not found and will not be enabled.                     [warning]
   imagefield was not found and will not be enabled.                    [warning]
   date was not found and will not be enabled.                          [warning]
   date_api was not found and will not be enabled.                      [warning]
   date_timezone was not found and will not be enabled.                 [warning]
   autoload was not found and will not be enabled.                      [warning]
   dbtng was not found and will not be enabled.                         [warning]
   cck was not found and will not be enabled.                           [warning]
   

   .
   These happen during the call to site-upgrade-modules.

7. My site-upgrade-modules call is returning FALSE so rollback ensues and I can't proceed.

~/htd/d6/sites/all$ drush pm-list --status=enabled --pipe --no-core
content
filefield
imagefield
date
date_api
date_timezone
autoload
dbtng

- the date_* modules come from the date project.
- dbtng is from http://drupal.org/project/dbtng and obviously that module is in core now.

i like the --uninstall option.

Not so sure about adding drupal-6. I think we should change what 6.x does. It should get latest recommended and then we expect folks to use --dev if they want snapshot. We can do that in a later patch so this one stays backportable.

Still need a hook in _drush_upgrade_special_projects() for commandfiles to alter the map.

For code comments, lets use start with a Capital and end with a period. Just change the ones you come across. I know that we are not consistent with this in this file or in many others in drush.

We can do --simulate in a follow up patch.

Will test later today.

This patch review is based on a couple patches ago ...

1. Now that I think of it, I think --uninstall needs docs clarification. One could assume we are uninstalling in the source database. But we are uninstalling during the twilight zone phase.
2. drush_upgrade_site_upgrade() is a giant function. We need to make it more grokkable. One idea is to split it out into helper functions for each step. If thats awkward for some reason, we should summarize what it does in its Doxygen.
3. We have a bug somewhere in string replacement:
   

   About to perform updatedb for Drupal core on @sup7                   [ok]
   executing !cmd                                                          [notice]
   

4. You are using the project imagefield, which requires data migration
   or other special processing.  Data migration for this module will be
   provided by the Content Migrate submodule of cck.

   . We should be more clear about what we expect admin to do. I guess they need to enable content_migrate *after* sup has run?

5. If this happens, disable and uninstall the date_api module before running site-upgrade.. We can recommend --uninstall instead now.

6. Undefined variable: destination_conf_path in                            [notice]
   /Users/mw/c/h/drush/commands/core/upgrade.drush.inc on line 155.
   

7. I got delayed for a long while by errors. The root cause was that my site was using CVS checkouts for modules but not cvs_deploy module. I think pm commands recognize this condition. Maybe we should also do that. Low priority.
   
8. The call to site-upgrade-modules passes date_api as an argument despite it being on the --uninstall list.
9. Please use --no-prefix when generating diffs from git.

Tested it out and am happy enough with it. Feel free to commit and keep working on trimming down that giant function and other todos. I will commit soon if greg does not. On my way to Chicago now.

1. Lets add 'default-major' to drushrc.php as i think it is useful for folks who are still on 6.
2. Remove '* @todo Upgrade to specific releases.'
3. We need a topic for this command to explain what it does in detail

Reopening for a few minor things:

1. Further refactor large callback function (move WARN section)
2. The project drupal has no releases in version 7 . We don't remove drupal from the array of contrib projects.
3. Undefined index: name in sitealias.inc on line 664.. $all_site_aliases['@self'] has no 'name'.

With the goal of having a script to call that takes a site through a full upgrade based on update code in the site repository, this Drush script re-implements most of site-upgrade without the downloads or the project status/warning messages:

http://drupal.org/sandbox/mlncn/1154002

Definitely want to refactor drush sup to be able to run this way, or to share code with a function that does this.

[UPDATE: ignore the sleep-deprived fool, this code works fine.] Also, this code in site-upgrade does not blow away the target database for me. I'm assuming it's something in my set-up (perhaps same issue as #1110044: How to tell site-install to use the database root user for creating the database?), but it's important for Drush to flag when it is not able to delete the current database.

  // Always blow away the target database so we start fresh.
  // We still have DRUSH_AFFIRMATIVE set from above, so this will not prompt.
  drush_set_option('create-db', TRUE);
  drush_include(DRUSH_BASE_PATH . '/commands/sql', 'sync.sql');
  drush_invoke('sql-sync', '@self', $target_key);

The last comment, aside from being off-topic, is wrong-- that code does wipe the database clean... when i look at the correct database.

And the script i posted is pretty unnecessary. This is probably what Greg meant in #11 and something like the below works quite well:

drush site-upgrade @anjali.dev7 --reuse --core-only --uninstall=hashcash,devel
drush @anjali.dev7 site-upgrade-modules

An option that we could pass into site-upgrade in place of core-only that a) skips the contrib module recommendations step (as core only does) and b) removes the download list before calling site-upgrade-modules would be ideal. The two-step approach here loses the modules to enable step but i sort of want to script manually that anyway.

I would like to do what the above two commands do in one step. If the full site-upgrade has already been run once, all the modules we want in code have already been suggested and downloaded-- all that should be skipped.

Essentially a --reuse-ALL option that acts like --core-only except that it calls site-upgrade-modules with the list of previously disabled modules to re-enable.

I think i would be able to do a patch that extends the if statements, but first, does anyone have a use case for the present behavior of --reuse (which honors core but acts like contrib is still an open question), or can i simply change its behavior to expect to reuse all present code?

Yes, i'm using --core-only and then site-upgrade-modules as a separate command to work around a bug where even though the --reuse flag is given, a number of prompts are given related to contrib modules.

Based on the contrib modules enabled in this site, it is possible that[warning]
the site-upgrade command might fail.  See warnings above.
Would you like to continue anyway? (y/n):

If i'm re-using code, i don't want that prompt. More importantly, if i'm re-using code, i don't want it to ask me if i want to download every contrib module. I already have them. It shouldn't even try. And boy, does it try:

There is no recommended release for project activitystream.
Choose one of the available releases:
 [0]  :  Cancel                                                            
 [1]  :  7.x-2.x-dev  -  2011-Apr-16  -  Supported, Development, Installed 

Install location /home/ben/code/anjali7/web/sites/all/modules/contrib/activitystream already exists. Do you want to overwrite it? (y/n): 

Would a patch giving --reuse this meaning, no attempts to check or download contrib code, be accepted?

Greg, any update on when this will be in the master branch?

Thanks for the info.

• I do not have and never have used Git on either my development nor production servers, so I need to learn how to do that. I see that we may need to install a “Git Deploy” module if we ever use Git, at least for actual Drupal Modules (which this would not qualify as). I have used Subversion (svn) to obtain Acquia Drupal, etc. It seems to be pretty similar. Is there a way to get the Master branch without using Git? My PHP installation included svn but not git, and pear doesn’t work right either so using it to get git is problematic.
• Re: #47 ¶3: “If you have the token module enabled in your D6 site (likely), you will get an error when you run the Drush site-upgrade command.” — what error message? We’ve been getting some very strange messages telling us that the “system” table does not exist in the destination database, yet it clearly does. Is this the error in question? I was wondering what was causing it and was about to start an Issue on that. Yes, we had Token enabled.
• site-upgrade supposedly does not touch the source Drupal site, yet it wound up disabling every non-Core Contrib module in the site, and all Themes (even Garland), pretty thoroughly (but thankfully not irreversibly) breaking the source site. I assume that it intended to do this temporarily, but the above-mentioned fatal error crashed it before it could re-enable the disabled modules and themes? If this is the case, just to be safe, I recommend that the next major version either by default (perhaps overridable with a switch) copy the entire source site and database to a temporary site before doing any part of the upgrade and using that temporary site as its upgrade source (so that the active original will never be altered in any way for any reason, even temporarily), or at least automatically put the site in Maintenance Mode while the upgrade is in progress and only take it out of Maintenance Mode on successful completion of those tasks that it is able to do.

Oops, forgot to mention: we don’t use the UUID module (didn’t even know it existed until I saw your post!), so that is not an issue for us.

That worked nicely! Thanks much! I still have some theme work to do and some D6-only modules to find replacements for (or do without), but the site is working in D7 now!