cipd: Add -tag option to 'create' and 'pkg-register' subcomands.

cipd: Add -tag option to 'create' and 'pkg-register' subcomands.

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

[nodir, 2015-05-07 04:19:02]

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

https://codereview.chromium.org/1130733007/diff/20001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/common.go:65: // ValidateInstanceTag returns error is a
string doesn't look like a valid tag.
s/is/if

https://codereview.chromium.org/1130733007/diff/20001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/common.go:67: chunks := strings.Split(t, ":")
strings.SplitN(t, ":", 2)

otherwise it is inconsistent with server-side check. On server a tag value is
allowed to have ":".

https://codereview.chromium.org/1130733007/diff/20001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/common.go:69: return fmt.Errorf("The string %q doesn't
look a tag (a key:value pair)", t)
typo: look like

https://codereview.chromium.org/1130733007/diff/20001/go/src/infra/tools/cipd...
File go/src/infra/tools/cipd/common_test.go (right):

https://codereview.chromium.org/1130733007/diff/20001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/common_test.go:82: So(ValidateInstanceTag("good:tag"),
ShouldBeNil)
very:good:tag

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

https://codereview.chromium.org/1130733007/diff/20001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/remote.go:376: }
I like the fact that you can define a struct in Go right in the func body

https://codereview.chromium.org/1130733007/diff/20001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/remote.go:385: return &pendingProcessingError{message:
reply.ErrorMessage}
nit: I think you can omit `message: ` in simple cases like this, where you
probably don't expect more fields in future

https://codereview.chromium.org/1130733007/diff/20001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/remote.go:389: return fmt.Errorf("Unexpected status when
attaching tags: %s", reply.Status)
I wonder if you should panic in this case. How do you decide whether you should
panic or return an error?

https://codereview.chromium.org/1130733007/diff/20001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/remote.go:429: err = ValidateInstanceTag(tag)
why would you write this code if never used?

https://codereview.chromium.org/1130733007/diff/20001/go/src/infra/tools/cipd...
File go/src/infra/tools/cipd/uploader.go (right):

https://codereview.chromium.org/1130733007/diff/20001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/uploader.go:143: // instance already existed before.
english: the instance

[Vadim Sh., 2015-05-07 05:07:59]

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

https://codereview.chromium.org/1130733007/diff/20001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/common.go:65: // ValidateInstanceTag returns error is a
string doesn't look like a valid tag.
On 2015/05/07 04:19:02, nodir wrote:
> s/is/if

Done.

https://codereview.chromium.org/1130733007/diff/20001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/common.go:67: chunks := strings.Split(t, ":")
On 2015/05/07 04:19:02, nodir wrote:
> strings.SplitN(t, ":", 2)
> 
> otherwise it is inconsistent with server-side check. On server a tag value is
> allowed to have ":".

Good catch.

https://codereview.chromium.org/1130733007/diff/20001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/common.go:69: return fmt.Errorf("The string %q doesn't
look a tag (a key:value pair)", t)
On 2015/05/07 04:19:02, nodir wrote:
> typo: look like

Done.

https://codereview.chromium.org/1130733007/diff/20001/go/src/infra/tools/cipd...
File go/src/infra/tools/cipd/common_test.go (right):

https://codereview.chromium.org/1130733007/diff/20001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/common_test.go:82: So(ValidateInstanceTag("good:tag"),
ShouldBeNil)
On 2015/05/07 04:19:02, nodir wrote:
> very:good:tag

Done.

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

https://codereview.chromium.org/1130733007/diff/20001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/remote.go:385: return &pendingProcessingError{message:
reply.ErrorMessage}
On 2015/05/07 04:19:02, nodir wrote:
> nit: I think you can omit `message: ` in simple cases like this, where you
> probably don't expect more fields in future

Done. I forget about this "feature".

https://codereview.chromium.org/1130733007/diff/20001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/remote.go:389: return fmt.Errorf("Unexpected status when
attaching tags: %s", reply.Status)
On 2015/05/07 04:19:02, nodir wrote:
> I wonder if you should panic in this case. How do you decide whether you
should
> panic or return an error?

Simple: never panic. Once you start panicing, you'd need to use rescue to catch
panics. It introduces alternative control flow. I prefer panics to happen only
on really-really unexpected conditions and cause stack dump.

https://codereview.chromium.org/1130733007/diff/20001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/remote.go:429: err = ValidateInstanceTag(tag)
On 2015/05/07 04:19:02, nodir wrote:
> why would you write this code if never used?

It will be used soonish for detachTags call. I can remove it if you prefer.

https://codereview.chromium.org/1130733007/diff/20001/go/src/infra/tools/cipd...
File go/src/infra/tools/cipd/uploader.go (right):

https://codereview.chromium.org/1130733007/diff/20001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/uploader.go:143: // instance already existed before.
On 2015/05/07 04:19:02, nodir wrote:
> english: the instance

Done.

[nodir, 2015-05-07 06:47:11]

lgtm

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

https://codereview.chromium.org/1130733007/diff/20001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/remote.go:389: return fmt.Errorf("Unexpected status when
attaching tags: %s", reply.Status)
On 2015/05/07 05:07:58, Vadim Sh. wrote:
> On 2015/05/07 04:19:02, nodir wrote:
> > I wonder if you should panic in this case. How do you decide whether you
> should
> > panic or return an error?
> 
> Simple: never panic. Once you start panicing, you'd need to use rescue to
catch
> panics. It introduces alternative control flow. I prefer panics to happen only
> on really-really unexpected conditions and cause stack dump.

But this is a really-really unexpected condition :)

[Vadim Sh., 2015-05-07 06:47:32]

The CQ bit was checked by vadimsh@chromium.org

[commit-bot: I haz the power, 2015-05-07 06:47:43]

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

[Vadim Sh., 2015-05-07 06:50:06]

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

https://codereview.chromium.org/1130733007/diff/20001/go/src/infra/tools/cipd...
go/src/infra/tools/cipd/remote.go:389: return fmt.Errorf("Unexpected status when
attaching tags: %s", reply.Status)
On 2015/05/07 06:47:11, nodir wrote:
> On 2015/05/07 05:07:58, Vadim Sh. wrote:
> > On 2015/05/07 04:19:02, nodir wrote:
> > > I wonder if you should panic in this case. How do you decide whether you
> > should
> > > panic or return an error?
> > 
> > Simple: never panic. Once you start panicing, you'd need to use rescue to
> catch
> > panics. It introduces alternative control flow. I prefer panics to happen
only
> > on really-really unexpected conditions and cause stack dump.
> 
> But this is a really-really unexpected condition :)

No, not really. I can totally see how server can add new error condition that
old clients don't know how to handle. Better to exit cleanly with error message
rather than die with panic.

[commit-bot: I haz the power, 2015-05-07 06:50:16]

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