Code review comment for lp://staging/~gary/charms/precise/juju-gui/update-for-juju-core

Revision history for this message
Gary Poster (gary) wrote :

Reviewers: mp+161322_code.launchpad.net,

Message:
Please take a look.

Description:
Add handling for a case important to Juju Core

Juju Core adds a .empty file to empty directories, which illustrated the
fact that the last branch needed to be a bit more defensive.

I plan to land this trivial but important change immediately.

https://code.launchpad.net/~gary/charms/precise/juju-gui/update-for-juju-core/+merge/161322

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/9002043/

Affected files:
   A [revision details]
   M hooks/install

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: <email address hidden>
+New revision: <email address hidden>

Index: hooks/install
=== modified file 'hooks/install'
--- hooks/install 2013-04-28 00:59:29 +0000
+++ hooks/install 2013-04-28 09:57:41 +0000
@@ -51,9 +51,13 @@
              try:
                  bootstrap_utils.run(filename)
              except OSError, e:
- # If the exec.d file does not exist or is not runnable,
- # assume we can recover. Log the problem and proceed.
- if e.errno in (errno.ENOENT, errno.EACCES):
+ # If the exec.d file does not exist or is not runnable or
+ # is not a directory, assume we can recover. Log the
problem
+ # and proceed. Note that Juju Core has a special need of
+ # errno.ENOTDIR because it apparently adds a ".empty" file
in
+ # empty charm directories, so trying to run
+ # ./exec.d/.empty/charm-pre-install will trigger that
error.
+ if e.errno in (errno.ENOENT, errno.EACCES, errno.ENOTDIR):
                      log('{}: {}'.format(e.strerror, filename))
                  else:
                      raise

« Back to merge proposal