cipd: Client can resolve tags to instance IDs.

cipd: Client can resolve tags to instance IDs.

Tags now can be used in 'pkg-fetch' and 'ensure' subcommands instead of instance
IDs.

R=nodir@chromium.org
BUG=481270
Committed: https://chromium.googlesource.com/infra/infra/+/09420ec2e960e1644a74c3f5ac58bed263b0cac8

[nodir, 2015-05-21 17:07:48]

How about "resolve" or "pkg-resolve" subcommand?

https://codereview.chromium.org/1145423002/diff/1/go/src/infra/tools/cipd/cli...
File go/src/infra/tools/cipd/client.go (right):

https://codereview.chromium.org/1145423002/diff/1/go/src/infra/tools/cipd/cli...
go/src/infra/tools/cipd/client.go:286: if err :=
common.ValidateInstanceID(version); err == nil {
if common.ValidateInstanceID(version) == nil  {

https://codereview.chromium.org/1145423002/diff/1/go/src/infra/tools/cipd/cli...
go/src/infra/tools/cipd/client.go:292: client.Log.Infof("cipd: resolving version
'%s' of '%s'...", version, packageName)
%q instead of '%s' ?

https://codereview.chromium.org/1145423002/diff/1/go/src/infra/tools/cipd/cli...
go/src/infra/tools/cipd/client.go:474: pin, err :=
client.ResolveVersion(tokens[0], tokens[1])
You may want to have batch resolve

https://codereview.chromium.org/1145423002/diff/1/go/src/infra/tools/cipd/com...
File go/src/infra/tools/cipd/common/common.go (right):

https://codereview.chromium.org/1145423002/diff/1/go/src/infra/tools/cipd/com...
go/src/infra/tools/cipd/common/common.go:85: if err := ValidateInstanceID(v);
err == nil {
if ValidateInstanceID(v) == nil && ValidateInstanceTag(v) == nil {
  return nil
}

https://codereview.chromium.org/1145423002/diff/1/go/src/infra/tools/cipd/rem...
File go/src/infra/tools/cipd/remote.go (right):

https://codereview.chromium.org/1145423002/diff/1/go/src/infra/tools/cipd/rem...
go/src/infra/tools/cipd/remote.go:195: InstanceID   string `json:"instance_id"`
I wish I warned you earlier, but from my small experience, I believe APIs should
be able to process many requests at a time. It happened in buildbucket with
heartbeats, build puts and build cancellations.

https://codereview.chromium.org/1145423002/diff/1/go/src/infra/tools/cipd/rem...
go/src/infra/tools/cipd/remote.go:216: err = fmt.Errorf("More than one instance
of package '%s' match version '%s'", packageName, version)
What one is supposed to do in this case? Neither a tag, nor instance can be
removed. Once a tag is ambiguous, it is forever.
Add another tag?

[Vadim Sh., 2015-05-21 18:00:37]

> How about "resolve" or "pkg-resolve" subcommand?

Done.

https://codereview.chromium.org/1145423002/diff/1/go/src/infra/tools/cipd/cli...
File go/src/infra/tools/cipd/client.go (right):

https://codereview.chromium.org/1145423002/diff/1/go/src/infra/tools/cipd/cli...
go/src/infra/tools/cipd/client.go:286: if err :=
common.ValidateInstanceID(version); err == nil {
On 2015/05/21 17:07:48, nodir wrote:
> if common.ValidateInstanceID(version) == nil  {

Done.

https://codereview.chromium.org/1145423002/diff/1/go/src/infra/tools/cipd/cli...
go/src/infra/tools/cipd/client.go:292: client.Log.Infof("cipd: resolving version
'%s' of '%s'...", version, packageName)
On 2015/05/21 17:07:48, nodir wrote:
> %q instead of '%s' ?

Done.

https://codereview.chromium.org/1145423002/diff/1/go/src/infra/tools/cipd/cli...
go/src/infra/tools/cipd/client.go:474: pin, err :=
client.ResolveVersion(tokens[0], tokens[1])
On 2015/05/21 17:07:48, nodir wrote:
> You may want to have batch resolve

Yeah, I know. In practice CIPD is installing 1-2 packages, so no big deal doing
it sequentially (considering it simplifies code).

https://codereview.chromium.org/1145423002/diff/1/go/src/infra/tools/cipd/com...
File go/src/infra/tools/cipd/common/common.go (right):

https://codereview.chromium.org/1145423002/diff/1/go/src/infra/tools/cipd/com...
go/src/infra/tools/cipd/common/common.go:85: if err := ValidateInstanceID(v);
err == nil {
On 2015/05/21 17:07:48, nodir wrote:
> if ValidateInstanceID(v) == nil && ValidateInstanceTag(v) == nil {
>   return nil
> }

:) Done.

https://codereview.chromium.org/1145423002/diff/1/go/src/infra/tools/cipd/rem...
File go/src/infra/tools/cipd/remote.go (right):

https://codereview.chromium.org/1145423002/diff/1/go/src/infra/tools/cipd/rem...
go/src/infra/tools/cipd/remote.go:195: InstanceID   string `json:"instance_id"`
On 2015/05/21 17:07:48, nodir wrote:
> I wish I warned you earlier, but from my small experience, I believe APIs
should
> be able to process many requests at a time. It happened in buildbucket with
> heartbeats, build puts and build cancellations.

1) CIPD is not latency critical, thus I do not optimize it for latency.
2) It's Go. It's very easy and cheap to parallelize requests when needed. I
think 10 parallel requests to /resolve? should finish around the same time as a
single batched request.

https://codereview.chromium.org/1145423002/diff/1/go/src/infra/tools/cipd/rem...
go/src/infra/tools/cipd/remote.go:216: err = fmt.Errorf("More than one instance
of package '%s' match version '%s'", packageName, version)
On 2015/05/21 17:07:48, nodir wrote:
> What one is supposed to do in this case? Neither a tag, nor instance can be
> removed. Once a tag is ambiguous, it is forever.
> Add another tag?

The answer I was planning for is:
1) Add ACLs to tags.
2) Allow important tags to be set only by automated systems (buildbot).
3) Make sure automated system set tags in a unique way, so there's no
duplicates.

Also tags can be detached (there's endpoints API for this). And in general I do
not plan for tags to be used as version specifiers often. Refs will be used most
of the time (I hope).

[nodir, 2015-05-21 18:15:06]

lgtm

https://codereview.chromium.org/1145423002/diff/1/go/src/infra/tools/cipd/rem...
File go/src/infra/tools/cipd/remote.go (right):

https://codereview.chromium.org/1145423002/diff/1/go/src/infra/tools/cipd/rem...
go/src/infra/tools/cipd/remote.go:216: err = fmt.Errorf("More than one instance
of package '%s' match version '%s'", packageName, version)
On 2015/05/21 18:00:37, Vadim Sh. wrote:
> On 2015/05/21 17:07:48, nodir wrote:
> > What one is supposed to do in this case? Neither a tag, nor instance can be
> > removed. Once a tag is ambiguous, it is forever.
> > Add another tag?
> 
> The answer I was planning for is:
> 1) Add ACLs to tags.
> 2) Allow important tags to be set only by automated systems (buildbot).
> 3) Make sure automated system set tags in a unique way, so there's no
> duplicates.
> 
> Also tags can be detached (there's endpoints API for this). And in general I
do
> not plan for tags to be used as version specifiers often. Refs will be used
most
> of the time (I hope).

Acknowledged.

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

https://codereview.chromium.org/1145423002/diff/40001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/apps/cipd/main.go:471: return
client.ResolveVersion(packageName, version)
It seems functions like this in this file are called only once. I don't think
they worth being standalone functions as opposed to be inlined in Run functions.

I'd write another CL to inline them all

[Vadim Sh., 2015-05-21 18:19:39]

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

https://codereview.chromium.org/1145423002/diff/40001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/apps/cipd/main.go:471: return
client.ResolveVersion(packageName, version)
On 2015/05/21 18:15:06, nodir wrote:
> It seems functions like this in this file are called only once. I don't think
> they worth being standalone functions as opposed to be inlined in Run
functions.
> 
> I'd write another CL to inline them all

They are not all like this. buildInstanceFile and inspectInstanceFile are called
from multiple places. For the sake of uniformity I prefer the rest of them be
separate too. Run(...) handles command line and error response, concrete
functions do the work.

[Vadim Sh., 2015-05-21 18:21:08]

The CQ bit was checked by vadimsh@chromium.org

[commit-bot: I haz the power, 2015-05-21 18:21:25]

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

[commit-bot: I haz the power, 2015-05-21 18:25:08]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/infra/infra/+/09420ec2e960e1644a74c3f5ac58b...