internal/relui/sign: return description in ArtifactSigningStatus

The StatusRunning and StatusFailed protobuf messages have a field named
Description, though previously its content wasn't accessible. Return it
to the caller and log it. This way we can figure out whether to keep it
or possibly remove it entirely if it stays unused.

It was useful during initial development and testing at least.

Also modify ArtifactSigningStatus to return a nil error when status
is NotFound, since it's a valid status that the caller can handle.

For golang/go#53632.

Change-Id: I7f9b038d86f985b71300150d1ccc700e209cdf2b
Reviewed-on: https://go-review.googlesource.com/c/build/+/444255
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Reviewed-by: Carlos Amedee <carlos@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Dmitri Shuralyov <dmitshur@golang.org>

internal/relui/sign/server.go

@@ -116,12 +116,10 @@ func (rs *SigningServer) do(ctx context.Context, req *protos.SigningRequest) (*p
 func (rs *SigningServer) SignArtifact(ctx context.Context, bt BuildType, objectURI []string) (jobID string, _ error) {
 	resp, err := rs.do(ctx, &protos.SigningRequest{
 		MessageId: uuid.NewString(),
-		RequestOneof: &protos.SigningRequest_Sign{
-			Sign: &protos.SignArtifactRequest{
-				BuildType: bt.proto(),
-				GcsUri:    objectURI,
-			},
-		},
+		RequestOneof: &protos.SigningRequest_Sign{Sign: &protos.SignArtifactRequest{
+			BuildType: bt.proto(),
+			GcsUri:    objectURI,
+		}},
 	})
 	if err != nil {
 		return "", err
@@ -135,42 +133,37 @@ func (rs *SigningServer) SignArtifact(ctx context.Context, bt BuildType, objectU
 }
 
 // ArtifactSigningStatus implements Service.
-func (rs *SigningServer) ArtifactSigningStatus(ctx context.Context, jobID string) (status Status, objectURI []string, err error) {
+func (rs *SigningServer) ArtifactSigningStatus(ctx context.Context, jobID string) (_ Status, desc string, objectURI []string, _ error) {
 	resp, err := rs.do(ctx, &protos.SigningRequest{
 		MessageId: uuid.NewString(),
-		RequestOneof: &protos.SigningRequest_Status{
-			Status: &protos.SignArtifactStatusRequest{JobId: jobID},
-		},
+		RequestOneof: &protos.SigningRequest_Status{Status: &protos.SignArtifactStatusRequest{
+			JobId: jobID,
+		}},
 	})
 	if err != nil {
-		return StatusUnknown, nil, err
+		return StatusUnknown, "", nil, err
 	}
 	switch t := resp.StatusOneof.(type) {
 	case *protos.SigningStatus_Completed:
-		status = StatusCompleted
-		objectURI = t.Completed.GetGcsUri()
+		return StatusCompleted, "", t.Completed.GetGcsUri(), nil
 	case *protos.SigningStatus_Failed:
-		status = StatusFailed
+		return StatusFailed, t.Failed.GetDescription(), nil, nil
 	case *protos.SigningStatus_NotFound:
-		status = StatusNotFound
-		err = fmt.Errorf("signing request not found for message=%q", resp.GetMessageId())
+		return StatusNotFound, fmt.Sprintf("signing job %q not found", jobID), nil, nil
 	case *protos.SigningStatus_Running:
-		status = StatusRunning
+		return StatusRunning, t.Running.GetDescription(), nil, nil
 	default:
-		return 0, nil, fmt.Errorf("unexpected response type %T for a status request", t)
+		return 0, "", nil, fmt.Errorf("unexpected response type %T for a status request", t)
 	}
-	return status, objectURI, err
 }
 
 // CancelSigning implements Service.
 func (rs *SigningServer) CancelSigning(ctx context.Context, jobID string) error {
 	_, err := rs.do(ctx, &protos.SigningRequest{
 		MessageId: uuid.NewString(),
-		RequestOneof: &protos.SigningRequest_Cancel{
-			Cancel: &protos.SignArtifactCancelRequest{
-				JobId: jobID,
-			},
-		},
+		RequestOneof: &protos.SigningRequest_Cancel{Cancel: &protos.SignArtifactCancelRequest{
+			JobId: jobID,
+		}},
 	})
 	return err
 }

internal/relui/sign/server_test.go

@@ -59,7 +59,7 @@ func TestUpdateSigningStatusError(t *testing.T) {
 		ctx, cancel := context.WithTimeout(ctx, 5*time.Second)
 		defer cancel()
 		go fakeSigningServerClient(t, ctx, client)
-		if _, _, err := server.ArtifactSigningStatus(ctx, "not-exist"); err == nil {
+		if _, _, _, err := server.ArtifactSigningStatus(ctx, "not-exist"); err == nil {
 			t.Fatalf("ArtifactSigningStatus(ctx, %q) = _, _, nil; want error", "not-exist")
 		}
 	})
@@ -72,11 +72,11 @@ func signRequestSeries(t *testing.T, ctx context.Context, id string, server *Sig
 	if err != nil {
 		t.Fatalf("SignArtifact(ctx, %q, %v, %q = %s; want no error", id, buildT, uri, err)
 	}
-	_, _, err = server.ArtifactSigningStatus(ctx, jobID)
+	_, _, _, err = server.ArtifactSigningStatus(ctx, jobID)
 	if err != nil {
 		t.Fatalf("ArtifactSigningStatus(%q) = %s; want no error", id, err)
 	}
-	_, _, err = server.ArtifactSigningStatus(ctx, jobID)
+	_, _, _, err = server.ArtifactSigningStatus(ctx, jobID)
 	if err != nil {
 		t.Fatalf("ArtifactSigningStatus(%q) = %s; want no error", id, err)
 	}

internal/relui/sign/sign.go

@@ -18,9 +18,10 @@ type Service interface {
 	// SignArtifact creates a request to sign a release artifact.
 	// The object URI must be URIs for file(s) on the service private GCS.
 	SignArtifact(ctx context.Context, bt BuildType, objectURI []string) (jobID string, _ error)
-	// ArtifactSigningStatus requests the status of an existing signing request message.
-	// If the message is at the status of completed then the objectURI will be populated with the URIs for signed files in GCS.
-	ArtifactSigningStatus(ctx context.Context, jobID string) (status Status, objectURI []string, err error)
+	// ArtifactSigningStatus retrieves the status of an existing signing request,
+	// or an error indicating that the status couldn't be determined.
+	// If the status is completed, objectURI will be populated with the URIs for signed files in GCS.
+	ArtifactSigningStatus(ctx context.Context, jobID string) (_ Status, description string, objectURI []string, _ error)
 	// CancelSigning marks a previous signing request as no longer needed,
 	// possibly allowing resources to be freed sooner than otherwise.
 	CancelSigning(ctx context.Context, jobID string) error

internal/relui/workflows.go

@@ -815,7 +815,7 @@ func (b *BuildReleaseTasks) signArtifacts(ctx *wf.TaskContext, bt sign.BuildType
 		return nil, err
 	}
 	outURLs, jobError := task.AwaitCondition(ctx, 30*time.Second, func() (out []string, done bool, _ error) {
-		status, out, err := b.SignService.ArtifactSigningStatus(ctx, jobID)
+		status, desc, out, err := b.SignService.ArtifactSigningStatus(ctx, jobID)
 		if err != nil {
 			ctx.Printf("ArtifactSigningStatus ran into a retryable communication error: %v\n", err)
 			return nil, false, nil
@@ -824,8 +824,14 @@ func (b *BuildReleaseTasks) signArtifacts(ctx *wf.TaskContext, bt sign.BuildType
 		case sign.StatusCompleted:
 			return out, true, nil // All done.
 		case sign.StatusFailed:
+			if desc != "" {
+				return nil, true, fmt.Errorf("signing attempt failed: %s", desc)
+			}
 			return nil, true, fmt.Errorf("signing attempt failed")
 		default:
+			if desc != "" {
+				ctx.Printf("still waiting: %s\n", desc)
+			}
 			return nil, false, nil // Still waiting.
 		}
 	})