Break out small functions and add godoc

FAB-16108 #done

Change-Id: I3768568869b653623272e0587d6de8586c476083
Signed-off-by: Matthew Sykes <sykesmat@us.ibm.com>

Author: sykesm

core/container/externalbuilder/externalbuilder.go

@@ -25,32 +25,33 @@ import (
 )
 
 var (
+	// DefaultEnvWhitelist enumerates the list of environment variables that are
+	// implicitly propagated to external builder and launcher commands.
 	DefaultEnvWhitelist = []string{"LD_LIBRARY_PATH", "LIBPATH", "PATH", "TMPDIR"}
-	logger              = flogging.MustGetLogger("chaincode.externalbuilder")
-)
 
-const MetadataFile = "metadata.json"
+	logger = flogging.MustGetLogger("chaincode.externalbuilder")
+)
 
+// BuildInfo contains metadata is that is saved to the local file system with the
+// assets generated by an external builder. This is used to associate build output
+// with the builder that generated it.
 type BuildInfo struct {
+	// BuilderName is the user provided name of the external builder.
 	BuilderName string `json:"builder_name"`
 }
 
+// A Detector is responsible for orchestrating the external builder detection and
+// build process.
 type Detector struct {
+	// DurablePath is the file system location where chaincode assets are persisted.
 	DurablePath string
-	Builders    []*Builder
+	// Builders are the builders that detect and build processing will use.
+	Builders []*Builder
 }
 
-func (d *Detector) Detect(buildContext *BuildContext) *Builder {
-	for _, builder := range d.Builders {
-		if builder.Detect(buildContext) {
-			return builder
-		}
-	}
-	return nil
-}
-
-// CachedBuild returns a build instance that was already built, or nil, or
-// when an unexpected error is encountered, an error.
+// CachedBuild returns a build instance that was already built or nil when no
+// instance has been found.  An error is returned only when an unexpected
+// condition is encountered.
 func (d *Detector) CachedBuild(ccid string) (*Instance, error) {
 	durablePath := filepath.Join(d.DurablePath, SanitizeCCIDPath(ccid))
 	_, err := os.Stat(durablePath)
@@ -67,9 +68,8 @@ func (d *Detector) CachedBuild(ccid string) (*Instance, error) {
 		return nil, errors.WithMessagef(err, "could not read '%s' for build info", buildInfoPath)
 	}
 
-	buildInfo := &BuildInfo{}
-	err = json.Unmarshal(buildInfoData, buildInfo)
-	if err != nil {
+	var buildInfo BuildInfo
+	if err := json.Unmarshal(buildInfoData, &buildInfo); err != nil {
 		return nil, errors.WithMessagef(err, "malformed build info at '%s'", buildInfoPath)
 	}
 
@@ -87,11 +87,16 @@ func (d *Detector) CachedBuild(ccid string) (*Instance, error) {
 	return nil, errors.Errorf("chaincode '%s' was already built with builder '%s', but that builder is no longer available", ccid, buildInfo.BuilderName)
 }
 
+// Build executes the external builder detect and build process.
+//
+// Before running the detect and build process, the detector first checks the
+// durable path for the results of a previous build for the provided package.
+// If found, the detect and build process is skipped and the existing instance
+// is returned.
 func (d *Detector) Build(ccid string, md *persistence.ChaincodePackageMetadata, codeStream io.Reader) (*Instance, error) {
+	// A small optimization: prevent exploding the build package out into the
+	// file system unless there are external builders defined.
 	if len(d.Builders) == 0 {
-		// A small optimization, especially while the launcher feature is under development
-		// let's not explode the build package out into the filesystem unless there are
-		// external builders to run against it.
 		return nil, nil
 	}
 
@@ -110,7 +115,7 @@ func (d *Detector) Build(ccid string, md *persistence.ChaincodePackageMetadata,
 	}
 	defer buildContext.Cleanup()
 
-	builder := d.Detect(buildContext)
+	builder := d.detect(buildContext)
 	if builder == nil {
 		logger.Debugf("no external builder detected for %s", ccid)
 		return nil, nil
@@ -128,7 +133,7 @@ func (d *Detector) Build(ccid string, md *persistence.ChaincodePackageMetadata,
 
 	err = os.Mkdir(durablePath, 0700)
 	if err != nil {
-		return nil, errors.WithMessagef(err, "could not create dir '%s' to persist build ouput", durablePath)
+		return nil, errors.WithMessagef(err, "could not create dir '%s' to persist build output", durablePath)
 	}
 
 	buildInfo, err := json.Marshal(&BuildInfo{
@@ -166,6 +171,17 @@ func (d *Detector) Build(ccid string, md *persistence.ChaincodePackageMetadata,
 	}, nil
 }
 
+func (d *Detector) detect(buildContext *BuildContext) *Builder {
+	for _, builder := range d.Builders {
+		if builder.Detect(buildContext) {
+			return builder
+		}
+	}
+	return nil
+}
+
+// BuildContext holds references to the various assets locations necessary to
+// execute the detect, build, release, and run programs for external builders
 type BuildContext struct {
 	CCID        string
 	Metadata    *persistence.ChaincodePackageMetadata
@@ -176,6 +192,11 @@ type BuildContext struct {
 	BldDir      string
 }
 
+// NewBuildContext creates the directories required to runt he external
+// build process and extracts the chaincode package assets.
+//
+// Users of the BuildContext must call Cleanup when the build process is
+// complete to remove the transient file system assets.
 func NewBuildContext(ccid string, md *persistence.ChaincodePackageMetadata, codePackage io.Reader) (bc *BuildContext, err error) {
 	scratchDir, err := ioutil.TempDir("", "fabric-"+SanitizeCCIDPath(ccid))
 	if err != nil {
@@ -229,12 +250,15 @@ func NewBuildContext(ccid string, md *persistence.ChaincodePackageMetadata, code
 	}, nil
 }
 
+// Cleanup removes the build context artifacts.
 func (bc *BuildContext) Cleanup() {
 	os.RemoveAll(bc.ScratchDir)
 }
 
 var pkgIDreg = regexp.MustCompile("[<>:\"/\\\\|\\?\\*&]")
 
+// SanitizeCCIDPath is used to ensure that special characters are removed from
+// file names.
 func SanitizeCCIDPath(ccid string) string {
 	return pkgIDreg.ReplaceAllString(ccid, "-")
 }
@@ -256,16 +280,18 @@ func writeMetadataFile(ccid string, md *persistence.ChaincodePackageMetadata, ds
 		return errors.Wrap(err, "failed to marshal build metadata into JSON")
 	}
 
-	return ioutil.WriteFile(filepath.Join(dst, MetadataFile), mdBytes, 0700)
+	return ioutil.WriteFile(filepath.Join(dst, "metadata.json"), mdBytes, 0700)
 }
 
+// A Builder is used to interact with an external chaincode builder and launcher.
 type Builder struct {
 	EnvWhitelist []string
 	Location     string
 	Logger       *flogging.FabricLogger
 	Name         string
 }
 
+// CreateBuilders will construct builders from the peer configuration.
 func CreateBuilders(builderConfs []peer.ExternalBuilder) []*Builder {
 	var builders []*Builder
 	for _, builderConf := range builderConfs {
@@ -279,11 +305,12 @@ func CreateBuilders(builderConfs []peer.ExternalBuilder) []*Builder {
 	return builders
 }
 
+// Detect runs the `detect` script.
 func (b *Builder) Detect(buildContext *BuildContext) bool {
 	detect := filepath.Join(b.Location, "bin", "detect")
 	cmd := b.NewCommand(detect, buildContext.SourceDir, buildContext.MetadataDir)
 
-	err := RunCommand(b.Logger, cmd)
+	err := b.runCommand(cmd)
 	if err != nil {
 		logger.Debugf("builder '%s' detect failed: %s", b.Name, err)
 		return false
@@ -292,18 +319,20 @@ func (b *Builder) Detect(buildContext *BuildContext) bool {
 	return true
 }
 
+// Build runs the `build` script.
 func (b *Builder) Build(buildContext *BuildContext) error {
 	build := filepath.Join(b.Location, "bin", "build")
 	cmd := b.NewCommand(build, buildContext.SourceDir, buildContext.MetadataDir, buildContext.BldDir)
 
-	err := RunCommand(b.Logger, cmd)
+	err := b.runCommand(cmd)
 	if err != nil {
 		return errors.Wrapf(err, "external builder '%s' failed", b.Name)
 	}
 
 	return nil
 }
 
+// Release runs the `release` script.
 func (b *Builder) Release(buildContext *BuildContext) error {
 	release := filepath.Join(b.Location, "bin", "release")
 
@@ -312,52 +341,58 @@ func (b *Builder) Release(buildContext *BuildContext) error {
 		b.Logger.Debugf("Skipping release step for '%s' as no release binary found", buildContext.CCID)
 		return nil
 	}
-
 	if err != nil {
 		return errors.WithMessagef(err, "could not stat release binary '%s'", release)
 	}
 
 	cmd := b.NewCommand(release, buildContext.BldDir, buildContext.ReleaseDir)
-
-	if err := RunCommand(b.Logger, cmd); err != nil {
+	err = b.runCommand(cmd)
+	if err != nil {
 		return errors.Wrapf(err, "builder '%s' release failed", b.Name)
 	}
 
 	return nil
 }
 
-// RunConfig is serialized to disk when launching
-type RunConfig struct {
+// runConfig is serialized to disk when launching.
+type runConfig struct {
 	CCID        string `json:"chaincode_id"`
 	PeerAddress string `json:"peer_address"`
-	ClientCert  string `json:"client_cert"` // PEM encoded client certifcate
+	ClientCert  string `json:"client_cert"` // PEM encoded client certificate
 	ClientKey   string `json:"client_key"`  // PEM encoded client key
 	RootCert    string `json:"root_cert"`   // PEM encoded peer chaincode certificate
 }
 
-func (b *Builder) Run(ccid, bldDir string, peerConnection *ccintf.PeerConnection) (*Session, error) {
-	lc := &RunConfig{
-		PeerAddress: peerConnection.Address,
-		CCID:        ccid,
+func newRunConfig(ccid string, peerConnection *ccintf.PeerConnection) runConfig {
+	var tlsConfig ccintf.TLSConfig
+	if peerConnection.TLSConfig != nil {
+		tlsConfig = *peerConnection.TLSConfig
 	}
 
-	if peerConnection.TLSConfig != nil {
-		lc.ClientCert = string(peerConnection.TLSConfig.ClientCert)
-		lc.ClientKey = string(peerConnection.TLSConfig.ClientKey)
-		lc.RootCert = string(peerConnection.TLSConfig.RootCert)
+	return runConfig{
+		PeerAddress: peerConnection.Address,
+		CCID:        ccid,
+		ClientCert:  string(tlsConfig.ClientCert),
+		ClientKey:   string(tlsConfig.ClientKey),
+		RootCert:    string(tlsConfig.RootCert),
 	}
+}
 
+// Run starts the `run` script and returns a Session that can be used to
+// signal it and wait for termination.
+func (b *Builder) Run(ccid, bldDir string, peerConnection *ccintf.PeerConnection) (*Session, error) {
 	launchDir, err := ioutil.TempDir("", "fabric-run")
 	if err != nil {
 		return nil, errors.WithMessage(err, "could not create temp run dir")
 	}
 
-	marshaledLC, err := json.Marshal(lc)
+	rc := newRunConfig(ccid, peerConnection)
+	marshaledRC, err := json.Marshal(rc)
 	if err != nil {
 		return nil, errors.WithMessage(err, "could not marshal run config")
 	}
 
-	if err := ioutil.WriteFile(filepath.Join(launchDir, "chaincode.json"), marshaledLC, 0600); err != nil {
+	if err := ioutil.WriteFile(filepath.Join(launchDir, "chaincode.json"), marshaledRC, 0600); err != nil {
 		return nil, errors.WithMessage(err, "could not write root cert")
 	}
 
@@ -377,6 +412,15 @@ func (b *Builder) Run(ccid, bldDir string, peerConnection *ccintf.PeerConnection
 	return sess, nil
 }
 
+// runCommand runs a command and waits for it to complete.
+func (b *Builder) runCommand(cmd *exec.Cmd) error {
+	sess, err := Start(b.Logger, cmd)
+	if err != nil {
+		return err
+	}
+	return sess.Wait()
+}
+
 // NewCommand creates an exec.Cmd that is configured to prune the calling
 // environment down to the environment variables specified in the external
 // builder's EnvironmentWhitelist and the DefaultEnvWhitelist.
@@ -408,11 +452,3 @@ func contains(envWhiteList []string, key string) bool {
 	}
 	return false
 }
-
-func RunCommand(logger *flogging.FabricLogger, cmd *exec.Cmd) error {
-	sess, err := Start(logger, cmd)
-	if err != nil {
-		return err
-	}
-	return sess.Wait()
-}

core/container/externalbuilder/externalbuilder_test.go

@@ -10,7 +10,6 @@ import (
 	"fmt"
 	"io/ioutil"
 	"os"
-	"os/exec"
 	"path/filepath"
 	"strings"
 
@@ -21,7 +20,6 @@ import (
 	"github.com/hyperledger/fabric/core/peer"
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
-	"github.com/onsi/gomega/gbytes"
 	"go.uber.org/zap"
 	"go.uber.org/zap/zapcore"
 )
@@ -131,7 +129,9 @@ var _ = Describe("externalbuilder", func() {
 
 			Context("when no builder can be found", func() {
 				BeforeEach(func() {
-					detector.Builders = nil
+					detector.Builders = externalbuilder.CreateBuilders([]peer.ExternalBuilder{
+						{Path: "bad1", Name: "bad1"},
+					})
 				})
 
 				It("returns a nil instance", func() {
@@ -157,7 +157,7 @@ var _ = Describe("externalbuilder", func() {
 
 				It("wraps and returns the error", func() {
 					_, err := detector.Build("fake-package-id", md, codePackage)
-					Expect(err).To(MatchError("could not create dir 'path/to/nowhere/fake-package-id' to persist build ouput: mkdir path/to/nowhere/fake-package-id: no such file or directory"))
+					Expect(err).To(MatchError("could not create dir 'path/to/nowhere/fake-package-id' to persist build output: mkdir path/to/nowhere/fake-package-id: no such file or directory"))
 				})
 			})
 		})
@@ -364,51 +364,6 @@ var _ = Describe("externalbuilder", func() {
 		})
 	})
 
-	Describe("RunCommand", func() {
-		var (
-			logger *flogging.FabricLogger
-			buf    *gbytes.Buffer
-		)
-
-		BeforeEach(func() {
-			buf = gbytes.NewBuffer()
-			enc := zapcore.NewConsoleEncoder(zapcore.EncoderConfig{MessageKey: "msg"})
-			core := zapcore.NewCore(enc, zapcore.AddSync(buf), zap.NewAtomicLevel())
-			logger = flogging.NewFabricLogger(zap.New(core).Named("logger"))
-		})
-
-		It("runs the command, directs stderr to the logger, and includes the command name", func() {
-			cmd := exec.Command("/bin/sh", "-c", `echo stdout && echo stderr >&2`)
-			err := externalbuilder.RunCommand(logger, cmd)
-			Expect(err).NotTo(HaveOccurred())
-			Expect(buf).To(gbytes.Say("stderr\t" + `{"command": "sh"}` + "\n"))
-		})
-
-		Context("when start fails", func() {
-			It("returns the error", func() {
-				cmd := exec.Command("nonsense-program")
-				err := externalbuilder.RunCommand(logger, cmd)
-				Expect(err).To(HaveOccurred())
-
-				execError, ok := err.(*exec.Error)
-				Expect(ok).To(BeTrue())
-				Expect(execError.Name).To(Equal("nonsense-program"))
-			})
-		})
-
-		Context("when the process exits with a non-zero return", func() {
-			It("returns the exec.ExitErr for the command", func() {
-				cmd := exec.Command("false")
-				err := externalbuilder.RunCommand(logger, cmd)
-				Expect(err).To(HaveOccurred())
-
-				exitErr, ok := err.(*exec.ExitError)
-				Expect(ok).To(BeTrue())
-				Expect(exitErr.ExitCode()).To(Equal(1))
-			})
-		})
-	})
-
 	Describe("SanitizeCCIDPath", func() {
 		It("forbids the set of forbidden windows characters", func() {
 			sanitizedPath := externalbuilder.SanitizeCCIDPath(`<>:"/\|?*&`)