cipd: Refactor client to make it more readable.

cipd: Refactor client to make it more readable.

Changes are:
 * Move functions that operate with files on disk to cipd/local subpackage.
 * Move small set of functions used by both cipd/local and cipd packages to
   cipd/common subpackage.
 * Introduce Client class that provides high level methods to work with packages
   (fetching, uploading, installing, etc). It replaces standalone functions that
   used to take a lot of arguments each. Now most of arguments are kept as
   Client state. As a consequence get rid of <opertaion>Options structs where
   makes sense.
 * Introduce Pin{} struct: a pair (package name, instance ID). It replaces
   several existing structs (PackageState, PackageInfo) and consistently used
   everywhere where package name & instance ID are used together. It was named
   "Pin" because: a) short word; b) doesn't collide with existing notions (such
   as ref or instance ID); c) implies concrete fixed version.
 * Extract Google Storage interaction in 'storageImpl' class.
 * Get rid of global vars mocked in tests (e.g. "clock" global var). Explicitly
   keep them in Client. Tests do not touch global environment now.
 * Consistently use single mockClient(...) method to mock HTTP calls
   (call expectations style). Also use Convey context object ("c C") to be able
   to properly raise assertions from non-main goroutines.
 * Always create http.Client lazily. Always use anonymous http.Client when
   talking to Google Storage (we use signed URLs, no need for authentication).

No changes to functionality.

R=nodir@chromium.org
BUG=

Committed: https://chromium.googlesource.com/infra/infra/+/0f5582b1836e7ef9348fa1e01cbfd6cc61836af5

[Vadim Sh., 2015-05-09 21:38:30]

Brace yourself, "34 files changed, 2264 insertions(+), 2432 deletions(-)" is
coming :(

Once I've realized how big this change is, it was already too difficult to split
it into smaller chunks.

[Vadim Sh., 2015-05-09 21:59:28]

The CQ bit was checked by vadimsh@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-05-09 21:59:44]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1129043003/20001

[nodir, 2015-05-12 19:09:21]

reviewed client.go

https://codereview.chromium.org/1129043003/diff/40001/go/src/infra/tools/cipd...
File go/src/infra/tools/cipd/client.go (right):

https://codereview.chromium.org/1129043003/diff/40001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/client.go:99: ErrBackendInaccessible =
errors.New("Request to the backend failed after multiple attempts")
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

https://codereview.chromium.org/1129043003/diff/40001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/client.go:105: // Client provides high level CIPD client
interface.
nit: high-level

https://codereview.chromium.org/1129043003/diff/40001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/client.go:107: // ServiceURL is root URL of the backend
service, or "" to use default service.
nit: the default service

https://codereview.chromium.org/1129043003/diff/40001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/client.go:122: // storage knows how upload and download
raw binaries using signed URLs.
nit: how to upload

https://codereview.chromium.org/1129043003/diff/40001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/client.go:125: // authClient is lazily created
http.Client to use to make authenticated requests.
revisit this comment

https://codereview.chromium.org/1129043003/diff/40001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/client.go:127: // anonClient is lazily created
http.Client to use to make anonymous requests.
this too

https://codereview.chromium.org/1129043003/diff/40001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/client.go:138: Role string
why not type Role? since you use a special type Action

https://codereview.chromium.org/1129043003/diff/40001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/client.go:174: }
Curious why clock is not here?

https://codereview.chromium.org/1129043003/diff/40001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/client.go:207: }
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

https://codereview.chromium.org/1129043003/diff/40001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/client.go:210: // together define access control list
for given package subpath.
the access control list for the given package subpath

https://codereview.chromium.org/1129043003/diff/40001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/client.go:226: func (client *Client) UploadToCAS(SHA1
string, data io.ReadSeeker, session *UploadSession) error {
why SHA1 is capital letters?

https://codereview.chromium.org/1129043003/diff/40001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/client.go:227: // Open new upload session if existing is
not provided.
an existing

https://codereview.chromium.org/1129043003/diff/40001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/client.go:261: if err != nil {
put err check before published

https://codereview.chromium.org/1129043003/diff/40001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/client.go:359: client.Log.Errorf("cipd: failed to fetch
%s (%s)", pin, err)
normally use use " - " between summary and error details

https://codereview.chromium.org/1129043003/diff/40001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/client.go:441: // Each line has a format "<package name>
<instance id>".
version, not instance id

https://codereview.chromium.org/1129043003/diff/40001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/client.go:443: return nil, makeError("expecting
'<package name> <instance id>' line")
here too

https://codereview.chromium.org/1129043003/diff/40001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/client.go:449: err =
common.ValidateInstanceID(tokens[1])
validate version?

https://codereview.chromium.org/1129043003/diff/40001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/client.go:462: // EnsurePackages is high level interface
for installation, removal and updates
nit: a high-level

https://codereview.chromium.org/1129043003/diff/40001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/client.go:465: // actions to bring the state of the site
root to desired one.
the desired one

https://codereview.chromium.org/1129043003/diff/40001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/client.go:542: errors = append(errors, err)
Consider prepending pin info to err message before putting to `errors`

https://codereview.chromium.org/1129043003/diff/40001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/client.go:550: return fmt.Errorf("Some actions failed:
%v", errors)
report errors on individual lines, otherwise it will look terrible

https://codereview.chromium.org/1129043003/diff/40001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/client.go:599: func buildActionPlan(desired
[]common.Pin, existing []common.Pin) (toDeploy []common.Pin, toDelete
[]common.Pin) {
toDeploy, toDelete []common.Pin

https://codereview.chromium.org/1129043003/diff/40001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/client.go:605: alreadyGood = e.InstanceID ==
d.InstanceID
build a map package name -> instanceId for `existing` because it may be big

[Vadim Sh., 2015-05-12 23:25:04]

https://codereview.chromium.org/1129043003/diff/40001/go/src/infra/tools/cipd...
File go/src/infra/tools/cipd/client.go (right):

https://codereview.chromium.org/1129043003/diff/40001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/client.go:99: ErrBackendInaccessible =
errors.New("Request to the backend failed after multiple attempts")
On 2015/05/12 19:09:20, nodir wrote:
> 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

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

https://codereview.chromium.org/1129043003/diff/40001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/client.go:105: // Client provides high level CIPD client
interface.
On 2015/05/12 19:09:20, nodir wrote:
> nit: high-level

Done.

https://codereview.chromium.org/1129043003/diff/40001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/client.go:107: // ServiceURL is root URL of the backend
service, or "" to use default service.
On 2015/05/12 19:09:20, nodir wrote:
> nit: the default service

It wasn't correct. Removed.

https://codereview.chromium.org/1129043003/diff/40001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/client.go:122: // storage knows how upload and download
raw binaries using signed URLs.
On 2015/05/12 19:09:20, nodir wrote:
> nit: how to upload

Done.

https://codereview.chromium.org/1129043003/diff/40001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/client.go:125: // authClient is lazily created
http.Client to use to make authenticated requests.
On 2015/05/12 19:09:20, nodir wrote:
> revisit this comment

What do you not like about this comment?

https://codereview.chromium.org/1129043003/diff/40001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/client.go:138: Role string
On 2015/05/12 19:09:20, nodir wrote:
> why not type Role? since you use a special type Action

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

https://codereview.chromium.org/1129043003/diff/40001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/client.go:174: }
On 2015/05/12 19:09:20, nodir wrote:
> Curious why clock is not here?

Just forgot

https://codereview.chromium.org/1129043003/diff/40001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/client.go:207: }
On 2015/05/12 19:09:20, nodir wrote:
> 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

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).

https://codereview.chromium.org/1129043003/diff/40001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/client.go:210: // together define access control list
for given package subpath.
On 2015/05/12 19:09:20, nodir wrote:
> the access control list for the given package subpath

Done.

https://codereview.chromium.org/1129043003/diff/40001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/client.go:226: func (client *Client) UploadToCAS(SHA1
string, data io.ReadSeeker, session *UploadSession) error {
On 2015/05/12 19:09:20, nodir wrote:
> why SHA1 is capital letters?

Done.

https://codereview.chromium.org/1129043003/diff/40001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/client.go:227: // Open new upload session if existing is
not provided.
On 2015/05/12 19:09:19, nodir wrote:
> an existing

Done.

https://codereview.chromium.org/1129043003/diff/40001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/client.go:261: if err != nil {
On 2015/05/12 19:09:19, nodir wrote:
> put err check before published

Done.

https://codereview.chromium.org/1129043003/diff/40001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/client.go:359: client.Log.Errorf("cipd: failed to fetch
%s (%s)", pin, err)
On 2015/05/12 19:09:20, nodir wrote:
> normally use use " - " between summary and error details

Done.

https://codereview.chromium.org/1129043003/diff/40001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/client.go:441: // Each line has a format "<package name>
<instance id>".
On 2015/05/12 19:09:19, nodir wrote:
> version, not instance id

Done.

https://codereview.chromium.org/1129043003/diff/40001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/client.go:443: return nil, makeError("expecting
'<package name> <instance id>' line")
On 2015/05/12 19:09:19, nodir wrote:
> here too

Done.

https://codereview.chromium.org/1129043003/diff/40001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/client.go:449: err =
common.ValidateInstanceID(tokens[1])
On 2015/05/12 19:09:20, nodir wrote:
> validate version?

See TODO, it is not implemented yet.

https://codereview.chromium.org/1129043003/diff/40001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/client.go:462: // EnsurePackages is high level interface
for installation, removal and updates
On 2015/05/12 19:09:19, nodir wrote:
> nit: a high-level

Done.

https://codereview.chromium.org/1129043003/diff/40001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/client.go:465: // actions to bring the state of the site
root to desired one.
On 2015/05/12 19:09:19, nodir wrote:
> the desired one

Done.

https://codereview.chromium.org/1129043003/diff/40001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/client.go:550: return fmt.Errorf("Some actions failed:
%v", errors)
On 2015/05/12 19:09:20, nodir wrote:
> report errors on individual lines, otherwise it will look terrible

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

https://codereview.chromium.org/1129043003/diff/40001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/client.go:599: func buildActionPlan(desired
[]common.Pin, existing []common.Pin) (toDeploy []common.Pin, toDelete
[]common.Pin) {
On 2015/05/12 19:09:19, nodir wrote:
> toDeploy, toDelete []common.Pin

Done.

https://codereview.chromium.org/1129043003/diff/40001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/client.go:605: alreadyGood = e.InstanceID ==
d.InstanceID
On 2015/05/12 19:09:20, nodir wrote:
> build a map package name -> instanceId for `existing` because it may be big

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:54]

https://codereview.chromium.org/1129043003/diff/40001/go/src/infra/tools/cipd...
File go/src/infra/tools/cipd/client.go (right):

https://codereview.chromium.org/1129043003/diff/40001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/client.go:125: // authClient is lazily created
http.Client to use to make authenticated requests.
On 2015/05/12 23:25:03, Vadim Sh. wrote:
> On 2015/05/12 19:09:20, nodir wrote:
> > revisit this comment
> 
> What do you not like about this comment?

This is better

https://codereview.chromium.org/1129043003/diff/40001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/client.go:138: Role string
On 2015/05/12 23:25:03, Vadim Sh. wrote:
> On 2015/05/12 19:09:20, nodir wrote:
> > why not type Role? since you use a special type Action
> 
> Action is conceptually an enum with only two valid options. I made it a
separate
> type to make it clearer.
> Role can be anything.

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

https://codereview.chromium.org/1129043003/diff/40001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/client.go:550: return fmt.Errorf("Some actions failed:
%v", errors)
On 2015/05/12 23:25:02, Vadim Sh. wrote:
> On 2015/05/12 19:09:20, nodir wrote:
> > report errors on individual lines, otherwise it will look terrible
> 
> Are multiline error objects allowed? Replaced this errors with
> ErrEnsurePackagesFailed. All details is in the log anyway.

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

https://codereview.chromium.org/1129043003/diff/40001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/client.go:605: alreadyGood = e.InstanceID ==
d.InstanceID
On 2015/05/12 23:25:03, Vadim Sh. wrote:
> On 2015/05/12 19:09:20, nodir wrote:
> > build a map package name -> instanceId for `existing` because it may be big
> 
> Done because it makes code shorter. As optimization it is quite useless since
we
> have 1-2 packages here in practice.

I expected it to be much more than 1-2

[Vadim Sh., 2015-05-12 23:48:46]

https://codereview.chromium.org/1129043003/diff/40001/go/src/infra/tools/cipd...
File go/src/infra/tools/cipd/client.go (right):

https://codereview.chromium.org/1129043003/diff/40001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/client.go:138: Role string
On 2015/05/12 23:35:53, nodir wrote:
> On 2015/05/12 23:25:03, Vadim Sh. wrote:
> > On 2015/05/12 19:09:20, nodir wrote:
> > > why not type Role? since you use a special type Action
> > 
> > Action is conceptually an enum with only two valid options. I made it a
> separate
> > type to make it clearer.
> > Role can be anything.
> 
> It is not anything, it is READER, WRITER, OWNER, isn't?

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:29]

https://codereview.chromium.org/1129043003/diff/40001/go/src/infra/tools/cipd...
File go/src/infra/tools/cipd/client.go (right):

https://codereview.chromium.org/1129043003/diff/40001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/client.go:138: Role string
On 2015/05/12 23:48:45, Vadim Sh. wrote:
> On 2015/05/12 23:35:53, nodir wrote:
> > On 2015/05/12 23:25:03, Vadim Sh. wrote:
> > > On 2015/05/12 19:09:20, nodir wrote:
> > > > why not type Role? since you use a special type Action
> > > 
> > > Action is conceptually an enum with only two valid options. I made it a
> > separate
> > > type to make it clearer.
> > > Role can be anything.
> > 
> > It is not anything, it is READER, WRITER, OWNER, isn't?
> 
> Yes, for now. With introduction of refs there will be new roles. For example,
> "canary:OWNER" role defines those who can move "canary" label.

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.

https://codereview.chromium.org/1129043003/diff/60001/go/src/infra/tools/cipd...
File go/src/infra/tools/cipd/apps/cipd/main.go (right):

https://codereview.chromium.org/1129043003/diff/60001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/apps/cipd/main.go:739: func
fetchInstanceFile(packageName, instanceID, instanceFile string, serviceOpts
ServiceOptions) error {
use pin here?

https://codereview.chromium.org/1129043003/diff/60001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/apps/cipd/main.go:818: logging.Infof("Instance ID:  %s",
inst.Pin().InstanceID)
Log standard packageName:instanceId string

https://codereview.chromium.org/1129043003/diff/60001/go/src/infra/tools/cipd...
File go/src/infra/tools/cipd/common/common.go (right):

https://codereview.chromium.org/1129043003/diff/60001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/common/common.go:57: func ValidatePin(pin Pin) error {
pin.Validate?

https://codereview.chromium.org/1129043003/diff/60001/go/src/infra/tools/cipd...
File go/src/infra/tools/cipd/local/deployer.go (right):

https://codereview.chromium.org/1129043003/diff/60001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/local/deployer.go:61: // possible. Returns information
about deployed instance ID.
Returns information about the deployed instance

https://codereview.chromium.org/1129043003/diff/60001/go/src/infra/tools/cipd...
File go/src/infra/tools/cipd/local/manifest.go (right):

https://codereview.chromium.org/1129043003/diff/60001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/local/manifest.go:32: func readManifest(r io.Reader)
(Manifest, error) {
nit: if you give names to return values, there will be less code in this func

https://codereview.chromium.org/1129043003/diff/60001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/local/manifest.go:46: func writeManifest(m *Manifest, w
io.Writer) error {
How do you judge whether a func should be a method or a func?

https://codereview.chromium.org/1129043003/diff/60001/go/src/infra/tools/cipd...
File go/src/infra/tools/cipd/remote.go (right):

https://codereview.chromium.org/1129043003/diff/60001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/remote.go:56: // etc. For now all errors that end up
here are considered retryable.
english: retriable

https://codereview.chromium.org/1129043003/diff/60001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/remote.go:120: if resp.StatusCode == 403 ||
resp.StatusCode == 401 {
move this if out of the enclosing if

https://codereview.chromium.org/1129043003/diff/60001/go/src/infra/tools/cipd...
File go/src/infra/tools/cipd/storage.go (right):

https://codereview.chromium.org/1129043003/diff/60001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/storage.go:66: offset = -1
errors += 1

[Vadim Sh., 2015-05-13 03:08:07]

https://codereview.chromium.org/1129043003/diff/40001/go/src/infra/tools/cipd...
File go/src/infra/tools/cipd/client.go (right):

https://codereview.chromium.org/1129043003/diff/40001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/client.go:138: Role string
On 2015/05/13 01:25:28, nodir wrote:
> On 2015/05/12 23:48:45, Vadim Sh. wrote:
> > On 2015/05/12 23:35:53, nodir wrote:
> > > On 2015/05/12 23:25:03, Vadim Sh. wrote:
> > > > On 2015/05/12 19:09:20, nodir wrote:
> > > > > why not type Role? since you use a special type Action
> > > > 
> > > > Action is conceptually an enum with only two valid options. I made it a
> > > separate
> > > > type to make it clearer.
> > > > Role can be anything.
> > > 
> > > It is not anything, it is READER, WRITER, OWNER, isn't?
> > 
> > Yes, for now. With introduction of refs there will be new roles. For
example,
> > "canary:OWNER" role defines those who can move "canary" label.
> 
> 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.

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

https://codereview.chromium.org/1129043003/diff/60001/go/src/infra/tools/cipd...
File go/src/infra/tools/cipd/apps/cipd/main.go (right):

https://codereview.chromium.org/1129043003/diff/60001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/apps/cipd/main.go:739: func
fetchInstanceFile(packageName, instanceID, instanceFile string, serviceOpts
ServiceOptions) error {
On 2015/05/13 01:25:29, nodir wrote:
> use pin here?

Yes, overlooked.

https://codereview.chromium.org/1129043003/diff/60001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/apps/cipd/main.go:818: logging.Infof("Instance ID:  %s",
inst.Pin().InstanceID)
On 2015/05/13 01:25:29, nodir wrote:
> Log standard packageName:instanceId string

Done.

https://codereview.chromium.org/1129043003/diff/60001/go/src/infra/tools/cipd...
File go/src/infra/tools/cipd/common/common.go (right):

https://codereview.chromium.org/1129043003/diff/60001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/common/common.go:57: func ValidatePin(pin Pin) error {
On 2015/05/13 01:25:29, nodir wrote:
> pin.Validate?

I'd prefer to keep it similar to other validators. Pin is POD-like type.

https://codereview.chromium.org/1129043003/diff/60001/go/src/infra/tools/cipd...
File go/src/infra/tools/cipd/local/deployer.go (right):

https://codereview.chromium.org/1129043003/diff/60001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/local/deployer.go:61: // possible. Returns information
about deployed instance ID.
On 2015/05/13 01:25:29, nodir wrote:
> Returns information about the deployed instance

Done.

https://codereview.chromium.org/1129043003/diff/60001/go/src/infra/tools/cipd...
File go/src/infra/tools/cipd/local/manifest.go (right):

https://codereview.chromium.org/1129043003/diff/60001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/local/manifest.go:32: func readManifest(r io.Reader)
(Manifest, error) {
On 2015/05/13 01:25:29, nodir wrote:
> nit: if you give names to return values, there will be less code in this func

Done.

https://codereview.chromium.org/1129043003/diff/60001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/local/manifest.go:46: func writeManifest(m *Manifest, w
io.Writer) error {
On 2015/05/13 01:25:29, nodir wrote:
> How do you judge whether a func should be a method or a func?

I flip a coin...

Basically, for immutable types I prefer functions. For stateful types - methods.

https://codereview.chromium.org/1129043003/diff/60001/go/src/infra/tools/cipd...
File go/src/infra/tools/cipd/remote.go (right):

https://codereview.chromium.org/1129043003/diff/60001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/remote.go:56: // etc. For now all errors that end up
here are considered retryable.
On 2015/05/13 01:25:29, nodir wrote:
> english: retriable

Changed to "temporary", retriable is not a word, I think...

https://codereview.chromium.org/1129043003/diff/60001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/remote.go:120: if resp.StatusCode == 403 ||
resp.StatusCode == 401 {
On 2015/05/13 01:25:29, nodir wrote:
> move this if out of the enclosing if

Done.

https://codereview.chromium.org/1129043003/diff/60001/go/src/infra/tools/cipd...
File go/src/infra/tools/cipd/storage.go (right):

https://codereview.chromium.org/1129043003/diff/60001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/storage.go:66: offset = -1
On 2015/05/13 01:25:29, nodir wrote:
> errors += 1

Done. Also deduplicated HTTP status code handling a bit and added HTTP 429
("Slow down") to the list of temporary errors (Google Storage was seen to return
it).

[nodir, 2015-05-13 15:54:25]

lgtm

https://codereview.chromium.org/1129043003/diff/60001/go/src/infra/tools/cipd...
File go/src/infra/tools/cipd/remote.go (right):

https://codereview.chromium.org/1129043003/diff/60001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/remote.go:56: // etc. For now all errors that end up
here are considered retryable.
On 2015/05/13 03:08:06, Vadim Sh. wrote:
> On 2015/05/13 01:25:29, nodir wrote:
> > english: retriable
> 
> Changed to "temporary", retriable is not a word, I think...

http://en.wiktionary.org/wiki/retriable

[Vadim Sh., 2015-05-13 21:58:01]

The CQ bit was checked by vadimsh@chromium.org

[commit-bot: I haz the power, 2015-05-13 21:58:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1129043003/80001

[commit-bot: I haz the power, 2015-05-13 22:02:39]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/infra/infra/+/0f5582b1836e7ef9348fa1e01cbfd...