cipd: Refactor client to make it more readable.

go/src/infra/tools/cipd/client.go

Index: go/src/infra/tools/cipd/client.go
diff --git a/go/src/infra/tools/cipd/client.go b/go/src/infra/tools/cipd/client.go
new file mode 100644
index 0000000000000000000000000000000000000000..55972b31e917ad9582a169836772d2e0bc515e90
--- /dev/null
+++ b/go/src/infra/tools/cipd/client.go
@@ -0,0 +1,629 @@
+// Copyright 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+/*
+Package cipd implements client side of Chrome Infra Package Deployer.
+
+TODO: write more.
+
+Binary package file format (in free form representation):
+  <binary package> := <zipped data>
+  <zipped data> := DeterministicZip(<all input files> + <manifest json>)
+  <manifest json> := File{
+    name: ".cipdpkg/manifest.json",
+    data: JSON({
+      "FormatVersion": "1",
+      "PackageName": <name of the package>
+    }),
+  }
+  DeterministicZip = zip archive with deterministic ordering of files and stripped timestamps
+
+Main package data (<zipped data> above) is deterministic, meaning its content
+depends only on inputs used to built it (byte to byte): contents and names of
+all files added to the package (plus 'executable' file mode bit) and a package
+name (and all other data in the manifest).
+
+Binary package data MUST NOT depend on a timestamp, hostname of machine that
+built it, revision of the source code it was built from, etc. All that
+information will be distributed as a separate metadata packet associated with
+the package when it gets uploaded to the server.
+
+TODO: expand more when there's server-side package data model (labels
+and stuff).
+*/
+package cipd
+
+import (
+	"bufio"
+	"errors"
+	"fmt"
+	"io"
+	"io/ioutil"
+	"net/http"
+	"os"
+	"path/filepath"
+	"strings"
+	"time"
+
+	"infra/libs/build"
+	"infra/libs/logging"
+
+	"infra/tools/cipd/common"
+	"infra/tools/cipd/local"
+)
+
+// PackageACLChangeAction defines a flavor of PackageACLChange.
+type PackageACLChangeAction string
+
+const (
+	// GrantRole is used in PackageACLChange to request a role to be granted.
+	GrantRole PackageACLChangeAction = "GRANT"
+	// RevokeRole is used in PackageACLChange to request a role to be revoked.
+	RevokeRole PackageACLChangeAction = "REVOKE"
+
+	// CASFinalizationTimeout is how long to wait for CAS service to finalize the upload.
+	CASFinalizationTimeout = 1 * time.Minute
+	// TagAttachTimeout is how long to wait for instance to be processed when attaching tags.
+	TagAttachTimeout = 1 * time.Minute
+
+	// UserAgent is HTTP user agent string for CIPD client.
+	UserAgent = "cipd 1.0"
+
+	// ProdServiceURL is URL of a backend to connect to if client is build with +release tag.
+	ProdServiceURL = "https://chrome-infra-packages.appspot.com"
+	// TestingServiceURL is URL of a backend to connect to if client is build without +release tag.
+	TestingServiceURL = "https://chrome-infra-packages-dev.appspot.com"
+)
+
+var (
+	// ErrFinalizationTimeout is returned if CAS service can not finalize upload fast enough.
+	ErrFinalizationTimeout = errors.New("Timeout while waiting for CAS service to finalize the upload")
+	// ErrBadUpload is returned when a package file is uploaded, but servers asks us to upload it again.
+	ErrBadUpload = errors.New("Package file is uploaded, but servers asks us to upload it again")
+	// ErrBadUploadSession is returned by UploadToCAS if provided UploadSession is not valid.
+	ErrBadUploadSession = errors.New("UploadURL must be set if UploadSessionID is used")
+	// ErrUploadSessionDied is returned by UploadToCAS if upload session suddenly disappeared.
+	ErrUploadSessionDied = errors.New("Upload session is unexpectedly missing")
+	// ErrNoUploadSessionID is returned by UploadToCAS if server didn't provide upload session ID.
+	ErrNoUploadSessionID = errors.New("Server didn't provide upload session ID")
+	// ErrAttachTagsTimeout is returned when service refuses to accept tags for a long time.
+	ErrAttachTagsTimeout = errors.New("Timeout while attaching tags")
+	// ErrDownloadError is returned by FetchInstance on download errors.
+	ErrDownloadError = errors.New("Failed to download the package file after multiple attempts")
+	// ErrUploadError is returned by RegisterInstance and UploadToCAS on upload errors.
+	ErrUploadError = errors.New("Failed to upload the package file after multiple attempts")
+	// ErrAccessDenined is returned by calls talking to backend on 401 or 403 HTTP errors.
+	ErrAccessDenined = errors.New("Access denied (not authenticated or not enough permissions)")
+	// ErrBackendInaccessible is returned by calls talking to backed if it doesn't response.
+	ErrBackendInaccessible = errors.New("Request to the backend failed after multiple attempts")

[nodir, 2015/05/12 19:09:20]

In general, it is cool to have each and every error defined, but I don't think
it is practical. No action asked for this comment

[Vadim Sh., 2015/05/12 23:25:03]

Some errors are not constants (e.g. errors that wrap other errors), and thus can
be predefined in 'var'.

+)
+
+// HTTPClientFactory lazily creates http.Client to use for making requests.
+type HTTPClientFactory func() (*http.Client, error)
+
+// Client provides high level CIPD client interface.

[nodir, 2015/05/12 19:09:20]

nit: high-level

[Vadim Sh., 2015/05/12 23:25:03]

Done.

+type Client struct {
+	// ServiceURL is root URL of the backend service, or "" to use default service.

[nodir, 2015/05/12 19:09:20]

nit: the default service

[Vadim Sh., 2015/05/12 23:25:02]

It wasn't correct. Removed.

+	ServiceURL string
+	// Log is a logger to use for logs, default is logging.DefaultLogger.
+	Log logging.Logger
+	// AuthenticatedClientFactory lazily creates http.Client to use for making RPC requests.
+	AuthenticatedClientFactory HTTPClientFactory
+	// AnonymousClientFactory lazily creates http.Client to use for making requests to storage.
+	AnonymousClientFactory HTTPClientFactory
+	// UserAgent is put into User-Agent HTTP header with each request.
+	UserAgent string
+
+	// clock provides current time and ability to sleep.
+	clock clock
+	// remote knows how to call backend REST API.
+	remote remote
+	// storage knows how upload and download raw binaries using signed URLs.

[nodir, 2015/05/12 19:09:20]

nit: how to upload

[Vadim Sh., 2015/05/12 23:25:03]

Done.

+	storage storage
+
+	// authClient is lazily created http.Client to use to make authenticated requests.

[nodir, 2015/05/12 19:09:20]

revisit this comment

[Vadim Sh., 2015/05/12 23:25:03]

What do you not like about this comment?

[nodir, 2015/05/12 23:35:54]

This is better

+	authClient *http.Client
+	// anonClient is lazily created http.Client to use to make anonymous requests.

[nodir, 2015/05/12 19:09:20]

this too

+	anonClient *http.Client
+}
+
+// PackageACL is per package path per role access control list that is a part of
+// larger overall ACL: ACL for package "a/b/c" is a union of PackageACLs for "a"
+// "a/b" and "a/b/c".
+type PackageACL struct {
+	// PackagePath is a package subpath this ACL is defined for.
+	PackagePath string
+	// Role is a role that listed users have, e.g. 'READER', 'WRITER', ...
+	Role string

[nodir, 2015/05/12 19:09:20]

why not type Role? since you use a special type Action

[Vadim Sh., 2015/05/12 23:25:03]

Action is conceptually an enum with only two valid options. I made it a separate
type to make it clearer.
Role can be anything.

[nodir, 2015/05/12 23:35:53]

It is not anything, it is READER, WRITER, OWNER, isn't?

[Vadim Sh., 2015/05/12 23:48:45]

Yes, for now. With introduction of refs there will be new roles. For example,
"canary:OWNER" role defines those who can move "canary" label.

[nodir, 2015/05/13 01:25:28]

IMO a role should not identify the resource. I think an ACL is (resource, role)
tuple, where role is a enum and denotes a list of allowed actions, and resource
identifies what is access-controlled. Currently there is only one type of
resources -- a package. With ref there will be a new resource type: a ref within
a package.

[Vadim Sh., 2015/05/13 03:08:06]

I'll consider this when I'll be implementing ACL for refs.

+	// Principals list users and groups granted the role.
+	Principals []string
+	// ModifiedBy specifies who modified the list the last time.
+	ModifiedBy string
+	// ModifiedTs is a timestamp when the list was modified the last time.
+	ModifiedTs time.Time
+}
+
+// PackageACLChange is a mutation to some package ACL.
+type PackageACLChange struct {
+	// Action defines what action to perform: GrantRole or RevokeRole.
+	Action PackageACLChangeAction
+	// Role to grant or revoke to a user or group.
+	Role string
+	// Principal is a user or a group to grant or revoke a role for.
+	Principal string
+}
+
+// UploadSession describes open CAS upload session.
+type UploadSession struct {
+	// ID identifies upload session in the backend.
+	ID string
+	// URL is where to upload the data to.
+	URL string
+}
+
+// NewClient initializes default CIPD client object. Its fields can be further
+// tweaked after this call.
+func NewClient() *Client {
+	c := &Client{
+		ServiceURL: ProdServiceURL,
+		Log:        logging.DefaultLogger,
+		AuthenticatedClientFactory: func() (*http.Client, error) { return http.DefaultClient, nil },
+		AnonymousClientFactory:     func() (*http.Client, error) { return http.DefaultClient, nil },
+		UserAgent:                  UserAgent,
+	}

[nodir, 2015/05/12 19:09:20]

Curious why clock is not here?

[Vadim Sh., 2015/05/12 23:25:03]

Just forgot

+	if !build.ReleaseBuild {
+		c.ServiceURL = TestingServiceURL
+		c.UserAgent += " testing"
+	}
+	c.clock = &clockImpl{}
+	c.remote = &remoteImpl{c}
+	c.storage = &storageImpl{c, uploadChunkSize}
+	return c
+}
+
+// doAuthenticatedHTTPRequest is used by remote implementation to make HTTP calls.
+func (client *Client) doAuthenticatedHTTPRequest(req *http.Request) (*http.Response, error) {
+	if client.authClient == nil {
+		var err error
+		client.authClient, err = client.AuthenticatedClientFactory()
+		if err != nil {
+			return nil, err
+		}
+	}
+	return client.authClient.Do(req)
+}
+
+// doAnonymousHTTPRequest is used by storage implementation to make HTTP calls.
+func (client *Client) doAnonymousHTTPRequest(req *http.Request) (*http.Response, error) {
+	if client.anonClient == nil {
+		var err error
+		client.anonClient, err = client.AnonymousClientFactory()
+		if err != nil {
+			return nil, err
+		}
+	}
+	return client.anonClient.Do(req)
+}

[nodir, 2015/05/12 19:09:20]

I wonder why here and in client factories, you have two things instead of one
with parameter? I think it would lease to less code

[Vadim Sh., 2015/05/12 23:25:03]

I think c.doAnonymousHTTPRequest(req) is more readable than c.doHTTPRequest(req,
true) ("true what?"). Introducing an enum is as almost as stupid as copy pasting
this few lines of code. I choose to copy paste few lines of code.

AnonymousClientFactory and AuthenticatedClientFactory are different because
generally callers never override AnonymousClientFactory: default one is
perfectly ok for any user. AuthenticatedClientFactory is being overridden with
correct implementation though (see main.go).

+
+// FetchACL returns a list of PackageACL objects (parent paths first) that
+// together define access control list for given package subpath.

[nodir, 2015/05/12 19:09:20]

the access control list for the given package subpath

[Vadim Sh., 2015/05/12 23:25:02]

Done.

+func (client *Client) FetchACL(packagePath string) ([]PackageACL, error) {
+	return client.remote.fetchACL(packagePath)
+}
+
+// ModifyACL applies a set of PackageACLChanges to a package path.
+func (client *Client) ModifyACL(packagePath string, changes []PackageACLChange) error {
+	return client.remote.modifyACL(packagePath, changes)
+}
+
+// UploadToCAS uploads package data blob to Content Addressed Store if it is not
+// there already. The data is addressed by SHA1 hash (also known as package's
+// InstanceID). It can be used as a standalone function (if 'session' is nil)
+// or as a part of more high level upload process (in that case upload session
+// can be opened elsewhere and its properties passed here via 'session'
+// argument). Returns nil on successful upload.
+func (client *Client) UploadToCAS(SHA1 string, data io.ReadSeeker, session *UploadSession) error {

[nodir, 2015/05/12 19:09:20]

why SHA1 is capital letters?

[Vadim Sh., 2015/05/12 23:25:03]

Done.

+	// Open new upload session if existing is not provided.

[nodir, 2015/05/12 19:09:19]

an existing

[Vadim Sh., 2015/05/12 23:25:03]

Done.

+	var err error
+	if session == nil {
+		client.Log.Infof("cipd: uploading %s: initiating", SHA1)
+		session, err = client.remote.initiateUpload(SHA1)
+		if err != nil {
+			client.Log.Warningf("cipd: can't upload %s - %s", SHA1, err)
+			return err
+		}
+		if session == nil {
+			client.Log.Infof("cipd: %s is already uploaded", SHA1)
+			return nil
+		}
+	} else {
+		if session.ID == "" || session.URL == "" {
+			return ErrBadUploadSession
+		}
+	}
+
+	// Upload the file to CAS storage.
+	err = client.storage.upload(session.URL, data)
+	if err != nil {
+		return err
+	}
+
+	// Finalize the upload, wait until server verifies and publishes the file.
+	started := client.clock.now()
+	delay := time.Second
+	for {
+		published, err := client.remote.finalizeUpload(session.ID)
+		if published {
+			client.Log.Infof("cipd: successfully uploaded %s", SHA1)
+			return nil
+		}
+		if err != nil {

[nodir, 2015/05/12 19:09:19]

put err check before published

[Vadim Sh., 2015/05/12 23:25:02]

Done.

+			client.Log.Warningf("cipd: upload of %s failed: %s", SHA1, err)
+			return err
+		}
+		if client.clock.now().Sub(started) > CASFinalizationTimeout {
+			client.Log.Warningf("cipd: upload of %s failed: timeout", SHA1)
+			return ErrFinalizationTimeout
+		}
+		client.Log.Infof("cipd: uploading - verifying")
+		client.clock.sleep(delay)
+		if delay < 4*time.Second {
+			delay += 500 * time.Millisecond
+		}
+	}
+}
+
+// RegisterInstance makes the package instance available for clients by
+// uploading it to the storage and registering it in the package repository.
+// 'instance' is a package instance to register.
+func (client *Client) RegisterInstance(instance local.PackageInstance) error {
+	// Attempt to register.
+	client.Log.Infof("cipd: registering %s", instance.Pin())
+	result, err := client.remote.registerInstance(instance.Pin())
+	if err != nil {
+		return err
+	}
+
+	// Asked to upload the package file to CAS first?
+	if result.uploadSession != nil {
+		err = client.UploadToCAS(instance.Pin().InstanceID, instance.DataReader(), result.uploadSession)
+		if err != nil {
+			return err
+		}
+		// Try again, now that file is uploaded.
+		client.Log.Infof("cipd: registering %s", instance.Pin())
+		result, err = client.remote.registerInstance(instance.Pin())
+		if err != nil {
+			return err
+		}
+		if result.uploadSession != nil {
+			return ErrBadUpload
+		}
+	}
+
+	if result.alreadyRegistered {
+		client.Log.Infof(
+			"cipd: instance %s is already registered by %s on %s",
+			instance.Pin(), result.registeredBy, result.registeredTs)
+	} else {
+		client.Log.Infof("cipd: instance %s was successfully registered", instance.Pin())
+	}
+
+	return nil
+}
+
+// AttachTagsWhenReady attaches tags to an instance retrying on "not yet processed" responses.
+func (client *Client) AttachTagsWhenReady(pin common.Pin, tags []string) error {
+	err := common.ValidatePin(pin)
+	if err != nil {
+		return err
+	}
+	if len(tags) == 0 {
+		return nil
+	}
+	for _, tag := range tags {
+		client.Log.Infof("cipd: attaching tag %s", tag)
+	}
+	deadline := client.clock.now().Add(TagAttachTimeout)
+	for client.clock.now().Before(deadline) {
+		err = client.remote.attachTags(pin, tags)
+		if err == nil {
+			client.Log.Infof("cipd: all tags attached")
+			return nil
+		}
+		if _, ok := err.(*pendingProcessingError); ok {
+			client.Log.Warningf("cipd: package instance is not ready yet - %s", err)
+			client.clock.sleep(5 * time.Second)
+		} else {
+			client.Log.Errorf("cipd: failed to attach tags - %s", err)
+			return err
+		}
+	}
+	client.Log.Errorf("cipd: failed to attach tags - deadline exceeded")
+	return ErrAttachTagsTimeout
+}
+
+// FetchInstance downloads package instance file from the repository.
+func (client *Client) FetchInstance(pin common.Pin, output io.WriteSeeker) error {
+	err := common.ValidatePin(pin)
+	if err != nil {
+		return err
+	}
+	client.Log.Infof("cipd: resolving fetch URL for %s", pin)
+	fetchInfo, err := client.remote.fetchInstance(pin)
+	if err == nil {
+		err = client.storage.download(fetchInfo.fetchURL, output)
+	}
+	if err != nil {
+		client.Log.Errorf("cipd: failed to fetch %s (%s)", pin, err)

[nodir, 2015/05/12 19:09:20]

normally use use " - " between summary and error details

[Vadim Sh., 2015/05/12 23:25:03]

Done.

+		return err
+	}
+	client.Log.Infof("cipd: successfully fetched %s", pin)
+	return nil
+}
+
+// FetchAndDeployInstance fetches the package instance and deploys it into
+// a site root. It doesn't check whether the instance is already deployed.
+func (client *Client) FetchAndDeployInstance(root string, pin common.Pin) error {
+	err := common.ValidatePin(pin)
+	if err != nil {
+		return err
+	}
+
+	// Use temp file for storing package file. Delete it when done.
+	var instance local.PackageInstance
+	tempPath := filepath.Join(root, local.SiteServiceDir, "tmp")
+	err = os.MkdirAll(tempPath, 0777)
+	if err != nil {
+		return err
+	}
+	f, err := ioutil.TempFile(tempPath, pin.InstanceID)
+	if err != nil {
+		return err
+	}
+	defer func() {
+		// Instance takes ownership of the file, no need to close it separately.
+		if instance == nil {
+			f.Close()
+		}
+		os.Remove(f.Name())
+	}()
+
+	// Fetch the package data to the provided storage.
+	err = client.FetchInstance(pin, f)
+	if err != nil {
+		return err
+	}
+
+	// Open the instance, verify the instance ID.
+	instance, err = local.OpenInstance(f, pin.InstanceID)
+	if err != nil {
+		return err
+	}
+	defer instance.Close()
+
+	// Deploy it. 'defer' will take care of removing the temp file if needed.
+	_, err = local.DeployInstance(root, instance)
+	return err
+}
+
+// ProcessEnsureFile parses text file that describes what should be installed
+// by EnsurePackages function. It is a text file where each line has a form:
+// <package name> <desired version>. Whitespaces are ignored. Lines that start
+// with '#' are ignored. Version can be specified as instance ID, tag or ref.
+// Will resolve tags and refs to concrete instance IDs.
+func (client *Client) ProcessEnsureFile(r io.Reader) ([]common.Pin, error) {
+	lineNo := 0
+	makeError := func(msg string) error {
+		return fmt.Errorf("Failed to parse desired state (line %d): %s", lineNo, msg)
+	}
+
+	out := []common.Pin{}
+	scanner := bufio.NewScanner(r)
+	for scanner.Scan() {
+		lineNo++
+
+		// Split each line into words, ignore white space.
+		tokens := []string{}
+		for _, chunk := range strings.Split(scanner.Text(), " ") {
+			chunk = strings.TrimSpace(chunk)
+			if chunk != "" {
+				tokens = append(tokens, chunk)
+			}
+		}
+
+		// Skip empty lines or lines starting with '#'.
+		if len(tokens) == 0 || tokens[0][0] == '#' {
+			continue
+		}
+
+		// Each line has a format "<package name> <instance id>".

[nodir, 2015/05/12 19:09:19]

version, not instance id

[Vadim Sh., 2015/05/12 23:25:03]

Done.

+		if len(tokens) != 2 {
+			return nil, makeError("expecting '<package name> <instance id>' line")

[nodir, 2015/05/12 19:09:19]

here too

[Vadim Sh., 2015/05/12 23:25:03]

Done.

+		}
+		err := common.ValidatePackageName(tokens[0])
+		if err != nil {
+			return nil, makeError(err.Error())
+		}
+		err = common.ValidateInstanceID(tokens[1])

[nodir, 2015/05/12 19:09:20]

validate version?

[Vadim Sh., 2015/05/12 23:25:02]

See TODO, it is not implemented yet.

+		if err != nil {
+			return nil, makeError(err.Error())
+		}
+
+		// Good enough.
+		out = append(out, common.Pin{PackageName: tokens[0], InstanceID: tokens[1]})
+	}
+
+	// TODO(vadimsh): Resolve tags to instance IDs.
+	return out, nil
+}
+
+// EnsurePackages is high level interface for installation, removal and updates

[nodir, 2015/05/12 19:09:19]

nit: a high-level

[Vadim Sh., 2015/05/12 23:25:03]

Done.

+// of packages inside some installation site root. Given a description of
+// what packages (and versions) should be installed it will do all necessary
+// actions to bring the state of the site root to desired one.

[nodir, 2015/05/12 19:09:19]

the desired one

[Vadim Sh., 2015/05/12 23:25:03]

Done.

+func (client *Client) EnsurePackages(root string, pins []common.Pin) error {
+	// Make sure a package is specified only once.
+	seen := make(map[string]bool, len(pins))
+	for _, p := range pins {
+		if seen[p.PackageName] {
+			return fmt.Errorf("Package %s is specified twice", p.PackageName)
+		}
+		seen[p.PackageName] = true
+	}
+
+	// Ensure site root is a directory (or missing).
+	root, err := filepath.Abs(filepath.Clean(root))
+	if err != nil {
+		return err
+	}
+	stat, err := os.Stat(root)
+	if err == nil && !stat.IsDir() {
+		return fmt.Errorf("Path %s is not a directory", root)
+	}
+	if err != nil && !os.IsNotExist(err) {
+		return err
+	}
+	rootExists := (err == nil)
+
+	// Enumerate existing packages (only if root already exists).
+	existing := []common.Pin{}
+	if rootExists {
+		existing, err = local.FindDeployed(root)
+		if err != nil {
+			client.Log.Errorf("Failed to enumerate installed packages: %s", err)
+			return err
+		}
+	}
+
+	// Figure out what needs to be updated and deleted, log it.
+	toDeploy, toDelete := buildActionPlan(pins, existing)
+	if len(toDeploy) == 0 && len(toDelete) == 0 {
+		client.Log.Infof("Everything is up-to-date.")
+		return nil
+	}
+	if len(toDeploy) != 0 {
+		client.Log.Infof("Packages to be installed:")
+		for _, pin := range toDeploy {
+			client.Log.Infof("  %s", pin)
+		}
+	}
+	if len(toDelete) != 0 {
+		client.Log.Infof("Packages to be removed:")
+		for _, pin := range toDelete {
+			client.Log.Infof("  %s", pin)
+		}
+	}
+
+	// Create the site root directory before installing anything there.
+	if len(toDeploy) != 0 && !rootExists {
+		err = os.MkdirAll(root, 0777)
+		if err != nil {
+			return err
+		}
+	}
+
+	// Remove all unneeded stuff.
+	errors := []error{}
+	for _, pin := range toDelete {
+		err = local.RemoveDeployed(root, pin.PackageName)
+		if err != nil {
+			client.Log.Errorf("Failed to remove %s - %s", pin.PackageName, err)
+			errors = append(errors, err)
+		}
+	}
+
+	// Install all new stuff.
+	for _, pin := range toDeploy {
+		err = client.FetchAndDeployInstance(root, pin)
+		if err != nil {
+			client.Log.Errorf("Failed to install %s - %s", pin, err)
+			errors = append(errors, err)

[nodir, 2015/05/12 19:09:20]

Consider prepending pin info to err message before putting to `errors`

+		}
+	}
+
+	if len(errors) == 0 {
+		client.Log.Infof("All changes applied.")
+		return nil
+	}
+	return fmt.Errorf("Some actions failed: %v", errors)

[nodir, 2015/05/12 19:09:20]

report errors on individual lines, otherwise it will look terrible

[Vadim Sh., 2015/05/12 23:25:02]

Are multiline error objects allowed? Replaced this errors with
ErrEnsurePackagesFailed. All details is in the log anyway.

[nodir, 2015/05/12 23:35:53]

This is fine. My concern was that %v with an array of errors would be hard to
read: no separation symbol

+}
+
+////////////////////////////////////////////////////////////////////////////////
+// Private structs and interfaces.
+
+type clock interface {
+	now() time.Time
+	sleep(time.Duration)
+}
+
+type remote interface {
+	fetchACL(packagePath string) ([]PackageACL, error)
+	modifyACL(packagePath string, changes []PackageACLChange) error
+
+	initiateUpload(sha1 string) (*UploadSession, error)
+	finalizeUpload(sessionID string) (bool, error)
+	registerInstance(pin common.Pin) (*registerInstanceResponse, error)
+
+	attachTags(pin common.Pin, tags []string) error
+	fetchInstance(pin common.Pin) (*fetchInstanceResponse, error)
+}
+
+type storage interface {
+	upload(url string, data io.ReadSeeker) error
+	download(url string, output io.WriteSeeker) error
+}
+
+type registerInstanceResponse struct {
+	uploadSession     *UploadSession
+	alreadyRegistered bool
+	registeredBy      string
+	registeredTs      time.Time
+}
+
+type fetchInstanceResponse struct {
+	fetchURL     string
+	registeredBy string
+	registeredTs time.Time
+}
+
+// Private stuff.
+
+type clockImpl struct{}
+
+func (c *clockImpl) now() time.Time        { return time.Now() }
+func (c *clockImpl) sleep(d time.Duration) { time.Sleep(d) }
+
+// buildActionPlan is used by EnsurePackages to figure out what to install or remove.
+func buildActionPlan(desired []common.Pin, existing []common.Pin) (toDeploy []common.Pin, toDelete []common.Pin) {

[nodir, 2015/05/12 19:09:19]

toDeploy, toDelete []common.Pin

[Vadim Sh., 2015/05/12 23:25:02]

Done.

+	// Figure out what needs to be installed or updated.
+	for _, d := range desired {
+		alreadyGood := false
+		for _, e := range existing {
+			if e.PackageName == d.PackageName {
+				alreadyGood = e.InstanceID == d.InstanceID

[nodir, 2015/05/12 19:09:20]

build a map package name -> instanceId for `existing` because it may be big

[Vadim Sh., 2015/05/12 23:25:03]

Done because it makes code shorter. As optimization it is quite useless since we
have 1-2 packages here in practice.

[nodir, 2015/05/12 23:35:53]

I expected it to be much more than 1-2

+				break
+			}
+		}
+		if !alreadyGood {
+			toDeploy = append(toDeploy, d)
+		}
+	}
+
+	// Figure out what needs to be removed.
+	for _, e := range existing {
+		keep := false
+		for _, d := range desired {
+			if e.PackageName == d.PackageName {
+				keep = true
+				break
+			}
+		}
+		if !keep {
+			toDelete = append(toDelete, e)
+		}
+	}
+
+	return
+}