Merge lp:~axwalk/juju-core/lp1293310-non-existent-hooks into lp:~go-bot/juju-core/trunk

[Andrew Wilkins]

worker/uniter: explicit LookPath for hooks

Go 1.3/tip changes os/exec.Command such that it
conditionally calls LookPath, based on whether or
not the specified path in is absolute. Since this
differs across versions, I have simply changed our
code to explicitly call LookPath all the time.

No tests changed; an existing test failed with Go
tip, and I verified the change fixes it.

Fixes lp:1293310

https://codereview.appspot.com/76780043/

worker/uniter: explicit LookPath for hooks

Go 1.3/tip changes os/exec.Command such that it
conditionally calls LookPath, based on whether or
not the specified path in is absolute. Since this
differs across versions, I have simply changed our
code to explicitly call LookPath all the time.

No tests changed; an existing test failed with Go
tip, and I verified the change fixes it.

Fixes lp:1293310

https://codereview.appspot.com/76780043/

[Andrew Wilkins (axwalk)]

Reviewers: mp+211247_code.launchpad.net,

Message:
Please take a look.

Description:
worker/uniter: explicit LookPath for hooks

Go 1.3/tip changes os/exec.Command such that it
conditionally calls LookPath, based on whether or
not the specified path in is absolute. Since this
differs across versions, I have simply changed our
code to explicitly call LookPath all the time.

No tests changed; an existing test failed with Go
tip, and I verified the change fixes it.

Fixes lp:1293310

https://code.launchpad.net/~axwalk/juju-core/lp1293310-non-existent-hooks/+merge/211247

(do not edit description out of merge proposal)

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

Affected files (+12, -8 lines):
   A [revision details]
   M worker/uniter/context.go

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: tarmac-20140316111812-jtsxq0bnl0ko1747
+New revision: <email address hidden>

Index: worker/uniter/context.go
=== modified file 'worker/uniter/context.go'
--- worker/uniter/context.go 2014-03-05 19:41:34 +0000
+++ worker/uniter/context.go 2014-03-17 05:49:29 +0000
@@ -253,7 +253,16 @@
  }

  func (ctx *HookContext) runCharmHook(hookName, charmDir string, env
[]string) error {
- ps := exec.Command(filepath.Join(charmDir, "hooks", hookName))
+ hook, err := exec.LookPath(filepath.Join(charmDir, "hooks", hookName))
+ if err != nil {
+ if ee, ok := err.(*exec.Error); ok && os.IsNotExist(ee.Err) {
+ // Missing hook is perfectly valid, but worth mentioning.
+ logger.Infof("skipped %q hook (not implemented)", hookName)
+ return &missingHookError{hookName}
+ }
+ return err
+ }
+ ps := exec.Command(hook)
   ps.Env = env
   ps.Dir = charmDir
   outReader, outWriter, err := os.Pipe()
@@ -274,13 +283,6 @@
    err = ps.Wait()
   }
   hookLogger.stop()
- if ee, ok := err.(*exec.Error); ok && err != nil {
- if os.IsNotExist(ee.Err) {
- // Missing hook is perfectly valid, but worth mentioning.
- logger.Infof("skipped %q hook (not implemented)", hookName)
- return &missingHookError{hookName}
- }
- }
   return err
  }

[John A Meinel (jameinel)]

LGTM

https://codereview.appspot.com/76780043/

=== modified file 'worker/uniter/context.go'
--- worker/uniter/context.go	2014-03-05 19:41:34 +0000
+++ worker/uniter/context.go	2014-03-17 05:55:13 +0000
@@ -253,7 +253,16 @@
 }
 
 func (ctx *HookContext) runCharmHook(hookName, charmDir string, env []string) error {
-	ps := exec.Command(filepath.Join(charmDir, "hooks", hookName))
+	hook, err := exec.LookPath(filepath.Join(charmDir, "hooks", hookName))
+	if err != nil {
+		if ee, ok := err.(*exec.Error); ok && os.IsNotExist(ee.Err) {
+			// Missing hook is perfectly valid, but worth mentioning.
+			logger.Infof("skipped %q hook (not implemented)", hookName)
+			return &missingHookError{hookName}
+		}
+		return err
+	}
+	ps := exec.Command(hook)
 	ps.Env = env
 	ps.Dir = charmDir
 	outReader, outWriter, err := os.Pipe()
@@ -274,13 +283,6 @@
 		err = ps.Wait()
 	}
 	hookLogger.stop()
-	if ee, ok := err.(*exec.Error); ok && err != nil {
-		if os.IsNotExist(ee.Err) {
-			// Missing hook is perfectly valid, but worth mentioning.
-			logger.Infof("skipped %q hook (not implemented)", hookName)
-			return &missingHookError{hookName}
-		}
-	}
 	return err
 }