Repository: helm/helm
File: pkg/action/hooks.go (Go)

Code before:
		// should always be set by this function. If we fail to do that for any reason, then HookPhaseUnknown is
		// the most appropriate value to surface.
		h.LastRun.Phase = release.HookPhaseUnknown

		// Create hook resources
		if _, err := cfg.KubeClient.Create(resources); err != nil {
			h.LastRun.CompletedAt = helmtime.Now()
			h.LastRun.Phase = release.HookPhaseFailed
			return errors.Wrapf(err, "warning: Hook %s %s failed", hook, h.Path)
		}

		// Watch hook resources until they have completed
		err = cfg.KubeClient.WatchUntilReady(resources, timeout)
		// Note the time of success/failure
		h.LastRun.CompletedAt = helmtime.Now()
		// Mark hook as succeeded or failed
		if err != nil {
			h.LastRun.Phase = release.HookPhaseFailed
			// If a hook is failed, check the annotation of the hook to determine if we should copy the logs client side
			if errOutputting := cfg.outputLogsByPolicy(h, rl.Namespace, release.HookOutputOnFailed); errOutputting != nil {
				// We log the error here as we want to propagate the hook failure upwards to the release object.
				log.Printf("error outputting logs for hook failure: %v", errOutputting)
			}
			// If a hook is failed, check the annotation of the hook to determine whether the hook should be deleted
			// under failed condition. If so, then clear the corresponding resource object in the hook
			if errDeleting := cfg.deleteHookByPolicy(h, release.HookFailed, timeout); err != nil {
				// We log the error here as we want to propagate the hook failure upwards to the release object.
				// This is a change in behaviour as the edge case previously would lose the hook error and only
				// raise the delete hook error.
				log.Printf("error the hook resource on hook failure: %v", errDeleting)
			}
			return err
		}
		h.LastRun.Phase = release.HookPhaseSucceeded
	}

	// If all hooks are successful, check the annotation of each hook to determine whether the hook should be deleted
	// or output should be logged under succeeded condition. If so, then clear the corresponding resource object in each hook
	for i := len(executingHooks) - 1; i >= 0; i-- {
		h := executingHooks[i]
		if err := cfg.outputLogsByPolicy(h, rl.Namespace, release.HookOutputOnSucceeded); err != nil {
			// We log here as we still want to attempt hook resource deletion even if output logging fails.
			log.Printf("error outputting logs for hook failure: %v", err)
		}
		if err := cfg.deleteHookByPolicy(h, release.HookSucceeded, timeout); err != nil {
			return err
		}
	}

	return nil
}

Review comment (bug):
Is this meant to be?
```suggestion
			if errDeleting := cfg.deleteHookByPolicy(h, release.HookFailed, timeout); errDeleting != nil {
```

Code after:
		// should always be set by this function. If we fail to do that for any reason, then HookPhaseUnknown is
		// the most appropriate value to surface.
		h.LastRun.Phase = release.HookPhaseUnknown

		// Create hook resources
		if _, err := cfg.KubeClient.Create(resources); err != nil {
			h.LastRun.CompletedAt = helmtime.Now()
			h.LastRun.Phase = release.HookPhaseFailed
			return errors.Wrapf(err, "warning: Hook %s %s failed", hook, h.Path)
		}

		// Watch hook resources until they have completed
		err = cfg.KubeClient.WatchUntilReady(resources, timeout)
		// Note the time of success/failure
		h.LastRun.CompletedAt = helmtime.Now()
		// Mark hook as succeeded or failed
		if err != nil {
			h.LastRun.Phase = release.HookPhaseFailed
			// If a hook is failed, check the annotation of the hook to determine if we should copy the logs client side
			if errOutputting := cfg.outputLogsByPolicy(h, rl.Namespace, release.HookOutputOnFailed); errOutputting != nil {
				// We log the error here as we want to propagate the hook failure upwards to the release object.
				log.Printf("error outputting logs for hook failure: %v", errOutputting)
			}
			// If a hook is failed, check the annotation of the hook to determine whether the hook should be deleted
			// under failed condition. If so, then clear the corresponding resource object in the hook
			if errDeleting := cfg.deleteHookByPolicy(h, release.HookFailed, timeout); errDeleting != nil {
				// We log the error here as we want to propagate the hook failure upwards to the release object.
				log.Printf("error deleting the hook resource on hook failure: %v", errDeleting)
			}
			return err
		}
		h.LastRun.Phase = release.HookPhaseSucceeded
	}

	// If all hooks are successful, check the annotation of each hook to determine whether the hook should be deleted
	// or output should be logged under succeeded condition. If so, then clear the corresponding resource object in each hook
	for i := len(executingHooks) - 1; i >= 0; i-- {
		h := executingHooks[i]
		if err := cfg.outputLogsByPolicy(h, rl.Namespace, release.HookOutputOnSucceeded); err != nil {
			// We log here as we still want to attempt hook resource deletion even if output logging fails.
			log.Printf("error outputting logs for hook failure: %v", err)
		}
		if err := cfg.deleteHookByPolicy(h, release.HookSucceeded, timeout); err != nil {
			return err
		}
	}

	return nil
}

// hookByWeight is a sorter for hooks