From: Edward Z. Yang <ezyang@mit.edu>
Date: Thu, 20 Aug 2009 16:34:13 +0000 (-0400)
Subject: Enhancements from our first migration.
X-Git-Url: https://scripts.mit.edu/gitweb/wizard.git/commitdiff_plain/d5a1592d7d682a4d453e05ac5cecea604f35f323

Enhancements from our first migration.

* Make MediaWiki regex more lenient with trailing newlines
* Add check to see if application is configured; if it isn't
  bail out and don't migrate.
* Perform increments early enough so that numeric IDs are unique
* Fix bug in increment printing
* fdopen() doesn't work, so simply create a blank lockfile
* Make force removal log message more descriptive
* Remove .scripts/old-version code
* Prefer git describe method, but if it doesn't work switch
  to .scripts-version
* Make shell return appropriate values during dry runs
* Make call error more descriptive (this messes with exception
  counting, however.)

Signed-off-by: Edward Z. Yang <ezyang@mit.edu>
---

diff --git a/TODO b/TODO
index e4d5cb6..547b5b1 100644
--- a/TODO
+++ b/TODO
@@ -7,6 +7,8 @@ TODO NOW:
 - Allow to migrate just one user (user filtering of installs, also
   has userland capabilities, although it means we need some way of
   selectively publishing the versions directory)
+- Make migrate script rollback if it's interrupted (especially if
+  by signal)
 
 - Make parallel-find.pl use `sudo -u username git describe --tags`
   to determine version.  Make parallel-find.pl have this have greater
@@ -16,6 +18,9 @@ TODO NOW:
   for mass-rollbacks).
 - Have the upgrader do locking (.scripts/lock, probably)
 
+- Relax MediaWiki regexes to terminate on semicolon, and not
+  require its own line.
+
 - Better error message if daemon/scripts-security-upd
   is not on scripts-security-upd list
 
diff --git a/wizard/app/mediawiki.py b/wizard/app/mediawiki.py
index 966a6a9..40358ce 100644
--- a/wizard/app/mediawiki.py
+++ b/wizard/app/mediawiki.py
@@ -5,7 +5,7 @@ from wizard import app, deploy, install, shell, util
 from wizard.app import php
 
 def make_filename_regex(var):
-    return 'LocalSettings.php', re.compile('^(\$' + app.expand_re(var) + r'''\s*=\s*)(.*)(;)$''', re.M)
+    return 'LocalSettings.php', re.compile('^(\$' + app.expand_re(var) + r'''\s*=\s*)(.*)(;)''', re.M)
 
 make_extractor = app.filename_regex_extractor(make_filename_regex)
 make_substitution = app.filename_regex_substitution(make_filename_regex)
@@ -41,6 +41,8 @@ class Application(deploy.Application):
         handler = install.ArgHandler("mysql", "admin", "email")
         handler.add(install.Arg("title", help="Title of your new MediaWiki install"))
         return handler
+    def checkConfig(self, deployment):
+        return os.path.isfile(os.path.join(deployment.location, "LocalSettings.php"))
     def install(self, options):
         try:
             os.unlink("LocalSettings.php")
diff --git a/wizard/command/mass_migrate.py b/wizard/command/mass_migrate.py
index 88f0087..ceaf549 100644
--- a/wizard/command/mass_migrate.py
+++ b/wizard/command/mass_migrate.py
@@ -47,6 +47,10 @@ def main(argv, baton):
             continue
         name = d.application.name
         if name != app: continue
+        # check if we want to punt due to --limit
+        i += 1
+        if options.limit and i > options.limit:
+            break
         if d.location in seen:
             continue
         # security check: see if the user's directory is the prefix of
@@ -54,13 +58,13 @@ def main(argv, baton):
         if not my_uid:
             uid = util.get_dir_uid(d.location)
             real = os.path.realpath(d.location)
-            if not real.startswith(pwd.getpwuid(uid).pw_dir + "/"):
-                logging.error("Security check failed, owner of deployment and owner of home directory mismatch for %s" % d.location)
+            try:
+                if not real.startswith(pwd.getpwuid(uid).pw_dir + "/"):
+                    logging.error("Security check failed, owner of deployment and owner of home directory mismatch for %s" % d.location)
+                    continue
+            except KeyError:
+                logging.error("Security check failed, could not look up owner of %s (uid %d)" % (d.location, uid))
                 continue
-        # check if we want to punt due to --limit
-        i += 1
-        if options.limit and i > options.limit:
-            break
         # calculate the log file, if a log dir was specified
         if options.log_dir:
             log_file = os.path.join(options.log_dir, shorten(i, d.location))
@@ -70,7 +74,7 @@ def main(argv, baton):
             def on_success(stdout, stderr):
                 if stderr:
                     warnings_log.write("%s\n" % d.location)
-                    logging.warning("Warnings [%04d] %s:\n%s" % (d.location, i, stderr))
+                    logging.warning("Warnings [%04d] %s:\n%s" % (i, d.location, stderr))
                 seen.add(d.location)
             def on_error(e):
                 if e.name == "wizard.command.migrate.AlreadyMigratedError" or \
diff --git a/wizard/command/migrate.py b/wizard/command/migrate.py
index 56b4e61..0adfefd 100644
--- a/wizard/command/migrate.py
+++ b/wizard/command/migrate.py
@@ -19,6 +19,10 @@ def main(argv, baton):
     logging.debug("uid is %d" % os.getuid())
 
     deployment = make_deployment() # uses chdir
+
+    if not deployment.configured:
+        raise NotConfiguredError(deployment.location)
+
     version = deployment.app_version
     repo    = version.application.repository(options.srv_path)
     tag     = version.scripts_tag
@@ -30,7 +34,7 @@ def main(argv, baton):
 
     try:
         try:
-            os.fdopen(os.open(".scripts-migrate-lock", os.O_CREAT | os.O_EXCL)).write(str(os.getpid()))
+            os.open(".scripts-migrate-lock", os.O_CREAT | os.O_EXCL)
         except OSError as e:
             if e.errno == errno.EEXIST:
                 raise DirectoryLockedError
@@ -93,13 +97,12 @@ def check_if_already_migrated(options):
                 name = "%s.%d" % (prefix, i)
                 if not os.path.exists(name):
                     break
+            logging.warning("Force removing %s directory (backup at %s)" % (file, name))
             os.rename(file, name)
         if has_git:
-            logging.warning("Force removing .git directory")
             if not options.dry_run:
                 rm_with_backup(".git")
         if has_scripts:
-            logging.warning("Force removing .scripts directory")
             if not options.dry_run:
                 rm_with_backup(".scripts")
 
@@ -193,6 +196,16 @@ migration, this means that the user was versioning their
 install using Git.  You cannot force this case.
 """
 
+class NotConfiguredError(Error):
+    def __init__(self, dir):
+        self.dir = dir
+    def __str__(self):
+        return """
+
+ERROR: The install was well-formed, but not configured
+(essential configuration files were not found.)
+"""
+
 class CorruptedAutoinstallError(Error):
     def __init__(self, dir):
         self.dir = dir
diff --git a/wizard/deploy.py b/wizard/deploy.py
index a923a0f..1b10204 100644
--- a/wizard/deploy.py
+++ b/wizard/deploy.py
@@ -12,7 +12,7 @@ import tempfile
 import logging
 
 import wizard
-from wizard import git, old_log, util
+from wizard import git, old_log, shell, util
 
 ## -- Global Functions --
 
@@ -106,6 +106,15 @@ class Deployment(object):
         is replaced with generic WIZARD_* variables.
         """
         return self.application.prepareConfig(self)
+    def checkConfig(self, deployment):
+        """
+        Checks if the application is configured.
+        """
+        raise NotImplemented
+    @property
+    def configured(self):
+        """Whether or not an autoinstall has been configured/installed for use."""
+        return self.application.checkConfig(self)
     @property
     def migrated(self):
         """Whether or not the autoinstalls has been migrated."""
@@ -124,10 +133,7 @@ class Deployment(object):
 
             Use of this is discouraged for migrated installs.
         """
-        if self.migrated:
-            return os.path.join(self.scripts_dir, 'old-version')
-        else:
-            return os.path.join(self.location, '.scripts-version')
+        return os.path.join(self.location, '.scripts-version')
     @property
     def version_file(self):
         """The absolute path of the ``.scripts/version`` file."""
@@ -157,11 +163,14 @@ class Deployment(object):
         """The :class:`ApplicationVersion` of this deployment."""
         if not self._app_version:
             if os.path.isdir(os.path.join(self.location, ".git")):
-                with util.ChangeDirectory(self.location):
-                    appname, _, version = git.describe().partition('-')
-                self._app_version = ApplicationVersion.make(appname, version)
-            else:
-                self._app_version = self.old_log[-1].version
+                try:
+                    with util.ChangeDirectory(self.location):
+                        appname, _, version = git.describe().partition('-')
+                    self._app_version = ApplicationVersion.make(appname, version)
+                except shell.CallError:
+                    pass
+        if not self._app_version:
+            self._app_version = self.old_log[-1].version
         return self._app_version
     @staticmethod
     def parse(line):
diff --git a/wizard/shell.py b/wizard/shell.py
index 36f2333..f034c93 100644
--- a/wizard/shell.py
+++ b/wizard/shell.py
@@ -92,7 +92,9 @@ class Shell(object):
         else:
             logging.info(msg)
         if self.dry:
-            return
+            if kwargs["strip"]:
+                return ''
+            return None, None
         if kwargs["python"] is None and is_python(args):
             kwargs["python"] = True
         if args[0] == "wizard":
@@ -325,7 +327,8 @@ class CallError(Error):
         self.stdout = stdout
         self.stderr = stderr
     def __str__(self):
-        return "CallError [%d]\n%s" % (self.code, self.stderr)
+        compact = self.stderr.rstrip().split("\n")[-1]
+        return "%s (exited with %d)\n%s" % (compact, self.code, self.stderr)
 
 class PythonCallError(CallError):
     """