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

go/src/infra/tools/cipd/remote.go

 // Copyright 2014 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
 
 import (
         "bytes"
         "encoding/json"
         "errors"
@@ skipping 49 matching lines @@
         Info     packageInstanceInfo
 }
 
 // roleChangeMsg corresponds to RoleChange proto message on backend.
 type roleChangeMsg struct {
         Action    string `json:"action"`
         Role      string `json:"role"`
         Principal string `json:"principal"`
 }
 
+// pendingProcessingError is returned by attachTags if package instance is not
+// yet ready and the call should be retried later.
+type pendingProcessingError struct {
+        message string
+}
+
+func (e *pendingProcessingError) Error() string {
+        return e.message
+}
+
 // newRemoteService is mocked in tests.
 var newRemoteService = func(client *http.Client, url string, log logging.Logger) *remoteService {
         log.Infof("cipd: service URL is %s", url)
         return &remoteService{
                 client:     client,
                 serviceURL: url,
                 log:        log,
         }
 }
 
@@ skipping 252 matching lines @@
         }
         switch reply.Status {
         case "SUCCESS":
                 return nil
         case "ERROR":
                 return errors.New(reply.ErrorMessage)
         }
         return fmt.Errorf("Unexpected reply status: %s", reply.Status)
 }
 
+func (r *remoteService) attachTags(packageName, instanceID string, tags []string) error {
+        // Tags will be passed in the request body, not via URL.
+        endpoint, err := tagsEndpoint(packageName, instanceID, nil)
+        if err != nil {
+                return err
+        }
+
+        if len(tags) == 0 {
+                return errors.New("At least one tag must be provided")
+        }
+        for _, tag := range tags {
+                err = ValidateInstanceTag(tag)
+                if err != nil {
+                        return err
+                }
+        }
+        var request struct {
+                Tags []string `json:"tags"`
+        }
+        request.Tags = tags
+
+        var reply struct {
+                Status       string `json:"status"`
+                ErrorMessage string `json:"error_message"`
+        }

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

I like the fact that you can define a struct in Go right in the func body

+        err = r.makeRequest(endpoint, "POST", &request, &reply)
+        if err != nil {
+                return err
+        }
+        switch reply.Status {
+        case "SUCCESS":
+                return nil
+        case "PROCESSING_NOT_FINISHED_YET":
+                return &pendingProcessingError{message: reply.ErrorMessage}

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

nit: I think you can omit `message: ` in simple cases like this, where you
probably don't expect more fields in future

[Vadim Sh., 2015/05/07 05:07:58]

Done. I forget about this "feature".

+        case "ERROR", "PROCESSING_FAILED":
+                return errors.New(reply.ErrorMessage)
+        }
+        return fmt.Errorf("Unexpected status when attaching tags: %s", reply.Status)

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

I wonder if you should panic in this case. How do you decide whether you should
panic or return an error?

[Vadim Sh., 2015/05/07 05:07:58]

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.

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

But this is a really-really unexpected condition :)

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

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.

+}
+
 ////////////////////////////////////////////////////////////////////////////////
 
 func instanceEndpoint(packageName, instanceID string) (string, error) {
         err := ValidatePackageName(packageName)
         if err != nil {
                 return "", err
         }
         err = ValidateInstanceID(instanceID)
         if err != nil {
                 return "", err
         }
         params := url.Values{}
         params.Add("package_name", packageName)
         params.Add("instance_id", instanceID)
         return "repo/v1/instance?" + params.Encode(), nil
 }
 
 func aclEndpoint(packagePath string) (string, error) {
         err := ValidatePackageName(packagePath)
         if err != nil {
                 return "", err
         }
         params := url.Values{}
         params.Add("package_path", packagePath)
         return "repo/v1/acl?" + params.Encode(), nil
 }
 
+func tagsEndpoint(packageName, instanceID string, tags []string) (string, error) {
+        err := ValidatePackageName(packageName)
+        if err != nil {
+                return "", err
+        }
+        err = ValidateInstanceID(instanceID)
+        if err != nil {
+                return "", err
+        }
+        for _, tag := range tags {
+                err = ValidateInstanceTag(tag)

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

why would you write this code if never used?

[Vadim Sh., 2015/05/07 05:07:58]

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

+                if err != nil {
+                        return "", err
+                }
+        }
+        params := url.Values{}
+        params.Add("package_name", packageName)
+        params.Add("instance_id", instanceID)
+        for _, tag := range tags {
+                params.Add("tag", tag)
+        }
+        return "repo/v1/tags?" + params.Encode(), nil
+}
+
 // convertTimestamp coverts string with int64 timestamp in microseconds since
 // to time.Time
 func convertTimestamp(ts string) (time.Time, error) {
         i, err := strconv.ParseInt(ts, 10, 64)
         if err != nil {
                 return time.Time{}, fmt.Errorf("Unexpected timestamp value '%s' in the server response", ts)
         }
         return time.Unix(0, i*1000), nil
 }
 
 func makePackageInstanceInfo(msg packageInstanceMsg) (pi packageInstanceInfo, err error) {
         ts, err := convertTimestamp(msg.RegisteredTs)
         if err != nil {
                 return
         }
         pi = packageInstanceInfo{
                 PackageName:  msg.PackageName,
                 InstanceID:   msg.InstanceID,
                 RegisteredBy: msg.RegisteredBy,
                 RegisteredTs: ts,
         }
         return
 }