chore(internal/librarian): check error field in docker response (#1565)

Fixes #1518

Author: JoeWang1127

generate_e2e_test.go

@@ -18,7 +18,6 @@
 package librarian
 
 import (
-	"encoding/json"
 	"fmt"
 	"math/rand"
 	"os"
@@ -51,8 +50,8 @@ func TestRunGenerate(t *testing.T) {
 			api:  "google/cloud/pubsub/v1",
 		},
 		{
-			name:    "non existent in api source",
-			api:     "google/non-existent/path",
+			name:    "failed due to simulated error in generate command",
+			api:     "google/cloud/future/v2",
 			wantErr: true,
 		},
 	} {
@@ -84,30 +83,19 @@ func TestRunGenerate(t *testing.T) {
 				if err == nil {
 					t.Fatalf("%s should fail", test.name)
 				}
+
+				// the exact message is not populated here, but we can check it's
+				// indeed an error returned from docker container.
+				if g, w := err.Error(), "exit status 1"; !strings.Contains(g, w) {
+					t.Fatalf("got %q, wanted it to contain %q", g, w)
+				}
+
 				return
 			}
+
 			if err != nil {
 				t.Fatalf("librarian generate command error = %v", err)
 			}
-
-			responseFile := filepath.Join(workRoot, "output", "generate-response.json")
-			if _, err := os.Stat(responseFile); err != nil {
-				t.Fatalf("can not find generate response, error = %v", err)
-			}
-
-			if test.wantErr {
-				data, err := os.ReadFile(responseFile)
-				if err != nil {
-					t.Fatalf("ReadFile() error = %v", err)
-				}
-				content := &genResponse{}
-				if err := json.Unmarshal(data, content); err != nil {
-					t.Fatalf("Unmarshal() error = %v", err)
-				}
-				if content.ErrorMessage == "" {
-					t.Fatalf("can not find error message in generate response")
-				}
-			}
 		})
 	}
 }
@@ -137,7 +125,7 @@ func TestRunConfigure(t *testing.T) {
 		{
 			name:         "failed due to simulated error in configure command",
 			api:          "google/cloud/another-library/v3",
-			library:      "simulate-configure-error-id",
+			library:      "simulate-command-error-id",
 			apiSource:    "testdata/e2e/configure/api_root",
 			updatedState: "testdata/e2e/configure/updated-state.yaml",
 			wantErr:      true,

internal/config/config.go

@@ -26,6 +26,9 @@ import (
 const (
 	// BuildRequest is a JSON file that describes which library to build/test.
 	BuildRequest = "build-request.json"
+	// BuildResponse is a JSON file that describes which library to change after
+	// built/test.
+	BuildResponse = "build-response.json"
 	// ConfigureRequest is a JSON file that describes which library to configure.
 	ConfigureRequest = "configure-request.json"
 	// ConfigureResponse is a JSON file that describes which library to change
@@ -36,6 +39,9 @@ const (
 	GeneratorInputDir = ".librarian/generator-input"
 	// GenerateRequest is a JSON file that describes which library to generate.
 	GenerateRequest = "generate-request.json"
+	// GenerateResponse is a JSON file that describes which library to change
+	// after re-generation.
+	GenerateResponse = "generate-response.json"
 	// LibrarianDir is the default directory to store librarian state/config files,
 	// along with any additional configuration.
 	LibrarianDir = ".librarian"

internal/config/state.go

@@ -99,6 +99,9 @@ type LibraryState struct {
 	// If not set, this defaults to the `source_roots`.
 	// A more specific `preserve_regex` takes precedence.
 	RemoveRegex []string `yaml:"remove_regex" json:"remove_regex"`
+	// An error message from the docker response.
+	// This field is ignored when writing to state.yaml.
+	ErrorMessage string `yaml:"-" json:"error,omitempty"`
 }
 
 var (

internal/docker/docker.go

@@ -152,7 +152,7 @@ func (c *Docker) Generate(ctx context.Context, request *GenerateRequest) error {
 	generatorInput := filepath.Join(request.RepoDir, config.GeneratorInputDir)
 	librarianDir := filepath.Join(request.RepoDir, config.LibrarianDir)
 	mounts := []string{
-		fmt.Sprintf("%s:/librarian:ro", librarianDir), // readonly volume
+		fmt.Sprintf("%s:/librarian", librarianDir),
 		fmt.Sprintf("%s:/input", generatorInput),
 		fmt.Sprintf("%s:/output", request.Output),
 		fmt.Sprintf("%s:/source:ro", request.ApiRoot), // readonly volume

internal/docker/docker_test.go

@@ -94,7 +94,7 @@ func TestDockerRun(t *testing.T) {
 			},
 			want: []string{
 				"run", "--rm",
-				"-v", fmt.Sprintf("%s/.librarian:/librarian:ro", repoDir),
+				"-v", fmt.Sprintf("%s/.librarian:/librarian", repoDir),
 				"-v", fmt.Sprintf("%s/.librarian/generator-input:/input", repoDir),
 				"-v", fmt.Sprintf("%s:/output", testOutput),
 				"-v", fmt.Sprintf("%s:/source:ro", testAPIRoot),
@@ -165,7 +165,7 @@ func TestDockerRun(t *testing.T) {
 			},
 			want: []string{
 				"run", "--rm",
-				"-v", fmt.Sprintf("%s/.librarian:/librarian:ro", repoDir),
+				"-v", fmt.Sprintf("%s/.librarian:/librarian", repoDir),
 				"-v", fmt.Sprintf("%s/.librarian/generator-input:/input", repoDir),
 				"-v", "localDir:/output",
 				"-v", fmt.Sprintf("%s:/source:ro", testAPIRoot),

internal/librarian/generate.go

@@ -273,6 +273,15 @@ func (r *generateRunner) runGenerateCommand(ctx context.Context, libraryID, outp
 		return "", err
 	}
 
+	// Read the library state from the response.
+	if _, err := readLibraryState(
+		func(data []byte, libraryState *config.LibraryState) error {
+			return json.Unmarshal(data, libraryState)
+		},
+		filepath.Join(generateRequest.RepoDir, config.LibrarianDir, config.GenerateResponse)); err != nil {
+		return "", err
+	}
+
 	if err := r.cleanAndCopyLibrary(libraryID, outputDir); err != nil {
 		return "", err
 	}
@@ -320,7 +329,18 @@ func (r *generateRunner) runBuildCommand(ctx context.Context, libraryID string)
 		RepoDir:   r.repo.GetDir(),
 	}
 	slog.Info("Build requested for library", "id", libraryID)
-	return r.containerClient.Build(ctx, buildRequest)
+	if err := r.containerClient.Build(ctx, buildRequest); err != nil {
+		return err
+	}
+
+	// Read the library state from the response.
+	_, err := readLibraryState(
+		func(data []byte, libraryState *config.LibraryState) error {
+			return json.Unmarshal(data, libraryState)
+		},
+		filepath.Join(buildRequest.RepoDir, config.LibrarianDir, config.BuildResponse))
+
+	return err
 }
 
 // clean removes files and directories from a root directory based on remove and preserve patterns.
@@ -530,7 +550,7 @@ func (r *generateRunner) runConfigureCommand(ctx context.Context) (string, error
 	}
 
 	// Read the new library state from the response.
-	libraryState, err := readConfigureResponse(
+	libraryState, err := readLibraryState(
 		func(data []byte, libraryState *config.LibraryState) error {
 			return json.Unmarshal(data, libraryState)
 		},

internal/librarian/generate_test.go

@@ -130,6 +130,40 @@ func TestRunBuildCommand(t *testing.T) {
 			build:     true,
 			container: &mockContainerClient{},
 		},
+		{
+			name:      "build with no response",
+			build:     true,
+			libraryID: "some-library",
+			repo:      newTestGitRepo(t),
+			state: &config.LibrarianState{
+				Libraries: []*config.LibraryState{
+					{
+						ID: "some-library",
+					},
+				},
+			},
+			container: &mockContainerClient{
+				noBuildResponse: true,
+			},
+			wantErr: true,
+		},
+		{
+			name:      "build with error response in response",
+			build:     true,
+			libraryID: "some-library",
+			repo:      newTestGitRepo(t),
+			state: &config.LibrarianState{
+				Libraries: []*config.LibraryState{
+					{
+						ID: "some-library",
+					},
+				},
+			},
+			container: &mockContainerClient{
+				wantErrorMsg: true,
+			},
+			wantErr: true,
+		},
 	} {
 		t.Run(test.name, func(t *testing.T) {
 			t.Parallel()
@@ -141,6 +175,7 @@ func TestRunBuildCommand(t *testing.T) {
 				state:           test.state,
 				containerClient: test.container,
 			}
+
 			err := r.runBuildCommand(context.Background(), test.libraryID)
 			if test.wantErr {
 				if err == nil {
@@ -170,7 +205,7 @@ func TestRunConfigureCommand(t *testing.T) {
 		wantErr            bool
 	}{
 		{
-			name: "configures library",
+			name: "configures library successfully",
 			api:  "some/api",
 			repo: newTestGitRepo(t),
 			state: &config.LibrarianState{
@@ -200,6 +235,24 @@ func TestRunConfigureCommand(t *testing.T) {
 			wantConfigureCalls: 1,
 			wantErr:            true,
 		},
+		{
+			name: "configures library with error message in response",
+			api:  "some/api",
+			repo: newTestGitRepo(t),
+			state: &config.LibrarianState{
+				Libraries: []*config.LibraryState{
+					{
+						ID:   "some-library",
+						APIs: []*config.API{{Path: "some/api"}},
+					},
+				},
+			},
+			container: &mockContainerClient{
+				wantErrorMsg: true,
+			},
+			wantConfigureCalls: 1,
+			wantErr:            true,
+		},
 		{
 			name: "configures library with no response",
 			api:  "some/api",
@@ -212,7 +265,9 @@ func TestRunConfigureCommand(t *testing.T) {
 					},
 				},
 			},
-			container:          &mockContainerClient{},
+			container: &mockContainerClient{
+				noConfigureResponse: true,
+			},
 			wantConfigureCalls: 1,
 			wantErr:            true,
 		},
@@ -249,7 +304,8 @@ func TestRunConfigureCommand(t *testing.T) {
 				containerClient: test.container,
 			}
 
-			if test.name == "configures library" {
+			if test.name == "configures library successfully" ||
+				test.name == "configures library with error message in response" {
 				if err := os.MkdirAll(filepath.Join(cfg.APISource, test.api), 0755); err != nil {
 					t.Fatal(err)
 				}
@@ -258,19 +314,6 @@ func TestRunConfigureCommand(t *testing.T) {
 				if err := os.WriteFile(filepath.Join(cfg.APISource, test.api, "example_service_v2.yaml"), data, 0755); err != nil {
 					t.Fatal(err)
 				}
-
-				// Write a configure-response.json because it is required by configure
-				// command.
-				if err := os.MkdirAll(filepath.Join(r.repo.GetDir(), config.LibrarianDir), 0755); err != nil {
-					t.Fatal(err)
-				}
-
-				libraryStr := fmt.Sprintf(`{
-	"ID": "%s"
-}`, test.state.Libraries[0].ID)
-				if err := os.WriteFile(filepath.Join(r.repo.GetDir(), config.LibrarianDir, config.ConfigureResponse), []byte(libraryStr), 0755); err != nil {
-					t.Fatal(err)
-				}
 			}
 
 			if test.name == "configures library with no response" ||
@@ -819,6 +862,28 @@ func TestGenerateScenarios(t *testing.T) {
 			build:     true,
 			wantErr:   true,
 		},
+		{
+			name:    "generate single existing library with error message in response",
+			api:     "some/api",
+			library: "some-library",
+			repo:    newTestGitRepo(t),
+			state: &config.LibrarianState{
+				Image: "gcr.io/test/image:v1.2.3",
+				Libraries: []*config.LibraryState{
+					{
+						ID:   "some-library",
+						APIs: []*config.API{{Path: "some/api"}},
+					},
+				},
+			},
+			container: &mockContainerClient{
+				wantErrorMsg: true,
+			},
+			ghClient:           &mockGitHubClient{},
+			wantGenerateCalls:  1,
+			wantConfigureCalls: 0,
+			wantErr:            true,
+		},
 		{
 			name: "generate all libraries configured in state",
 			repo: newTestGitRepo(t),