feat(internal/librarian): rework configure container contract (#898)

cuiy0006 · web-flow · commit ae87263b6044 · 2025-07-22T10:39:37.000-04:00
Refactor configure command container contract: Create a `generate-request.json` from state Mount `.librarian` to /librarian Mount `.librarian/generator-input` to /input Mount `apiRoot` to /source Fixes #769, fixes #815 --------- Signed-off-by: Yao Cui <cuiyao@google.com>
diff --git a/internal/config/config.go b/internal/config/config.go
@@ -25,7 +25,9 @@ import (
 
 const (
 	// BuildRequest is a JSON file that describes which library to build/test.
-	BuildRequest      string = "build-request.json"
+	BuildRequest string = "build-request.json"
+	// ConfigureRequest is a JSON file that describes which library to configure.
+	ConfigureRequest  string = "configure-request.json"
 	DefaultPushConfig string = "noreply-cloudsdk@google.com,Google Cloud SDK"
 	// GeneratorInputDir is the default directory to store files that generator
 	// needs to regenerate libraries from an empty directory.
diff --git a/internal/docker/docker.go b/internal/docker/docker.go
@@ -99,6 +99,23 @@ type BuildRequest struct {
 	RepoDir string
 }
 
+// ConfigureRequest contains all the information required for a language
+// container to run the configure command.
+type ConfigureRequest struct {
+	// cfg is a pointer to the [config.Config] struct, holding general configuration
+	// values parsed from flags or environment variables.
+	Cfg *config.Config
+	// state is a pointer to the [config.LibrarianState] struct, representing
+	// the overall state of the generation and release pipeline.
+	State *config.LibrarianState
+	// apiRoot specifies the root directory of the API specification repo.
+	ApiRoot string
+	// libraryID specifies the ID of the library to generate
+	LibraryID string
+	// RepoDir is the local root directory of the language repository.
+	RepoDir string
+}
+
 // New constructs a Docker instance which will invoke the specified
 // Docker image as required to implement language-specific commands,
 // providing the container with required environment variables.
@@ -177,22 +194,32 @@ func (c *Docker) Build(ctx context.Context, request *BuildRequest) error {
 }
 
 // Configure configures an API within a repository, either adding it to an
-// existing library or creating a new library. The API is indicated by the
-// apiPath directory within apiRoot, and the container is provided with the
-// generatorInput directory to record the results of configuration. The
-// library code is not generated.
-func (c *Docker) Configure(ctx context.Context, cfg *config.Config, apiRoot, apiPath, generatorInput string) error {
+// existing library or creating a new library.
+func (c *Docker) Configure(ctx context.Context, request *ConfigureRequest) error {
+	jsonFilePath := filepath.Join(request.RepoDir, config.LibrarianDir, config.ConfigureRequest)
+	if err := writeRequest(request.State, request.LibraryID, jsonFilePath); err != nil {
+		return err
+	}
+	defer func(name string) {
+		err := os.Remove(name)
+		if err != nil {
+			slog.Warn("fail to remove file", slog.String("name", name), slog.Any("err", err))
+		}
+	}(jsonFilePath)
 	commandArgs := []string{
-		"--source=/apis",
-		fmt.Sprintf("--%s=/%s", config.GeneratorInputDir, config.GeneratorInputDir),
-		fmt.Sprintf("--api=%s", apiPath),
+		"--librarian=/librarian",
+		"--input=/input",
+		"--source=/source",
+		fmt.Sprintf("--library-id=%s", request.LibraryID),
 	}
+	generatorInput := filepath.Join(request.RepoDir, config.GeneratorInputDir)
 	mounts := []string{
-		fmt.Sprintf("%s:/apis", apiRoot),
-		fmt.Sprintf("%s:/%s", generatorInput, config.GeneratorInputDir),
+		fmt.Sprintf("%s:/librarian", config.LibrarianDir),
+		fmt.Sprintf("%s:/input", generatorInput),
+		fmt.Sprintf("%s:/source:ro", request.ApiRoot), // readonly volume.
 	}
 
-	return c.runDocker(ctx, cfg, CommandConfigure, mounts, commandArgs)
+	return c.runDocker(ctx, request.Cfg, CommandConfigure, mounts, commandArgs)
 }
 
 func (c *Docker) runDocker(ctx context.Context, cfg *config.Config, command Command, mounts []string, commandArgs []string) (err error) {
diff --git a/internal/docker/docker_test.go b/internal/docker/docker_test.go
@@ -249,20 +249,65 @@ func TestDockerRun(t *testing.T) {
 				Image: testImage,
 			},
 			runCommand: func(ctx context.Context, d *Docker) error {
-				return d.Configure(ctx, cfg, testAPIRoot, testAPIPath, testGeneratorInput)
+				configureRequest := &ConfigureRequest{
+					Cfg:       cfg,
+					State:     state,
+					LibraryID: testLibraryID,
+					RepoDir:   ".",
+					ApiRoot:   testAPIRoot,
+				}
+				return d.Configure(ctx, configureRequest)
 			},
 			want: []string{
 				"run", "--rm",
-				"-v", fmt.Sprintf("%s:/apis", testAPIRoot),
-				"-v", fmt.Sprintf("%s:/.librarian/generator-input", testGeneratorInput),
+				"-v", ".librarian:/librarian",
+				"-v", ".librarian/generator-input:/input",
+				"-v", fmt.Sprintf("%s:/source:ro", testAPIRoot),
 				testImage,
 				string(CommandConfigure),
-				"--source=/apis",
-				"--.librarian/generator-input=/.librarian/generator-input",
-				fmt.Sprintf("--api=%s", testAPIPath),
+				"--librarian=/librarian",
+				"--input=/input",
+				"--source=/source",
+				fmt.Sprintf("--library-id=%s", testLibraryID),
 			},
 			wantErr: false,
 		},
+		{
+			name: "Configure with invalid repo dir",
+			docker: &Docker{
+				Image: testImage,
+			},
+			runCommand: func(ctx context.Context, d *Docker) error {
+				configureRequest := &ConfigureRequest{
+					Cfg:       cfg,
+					State:     state,
+					LibraryID: testLibraryID,
+					RepoDir:   "/non-exist-dir",
+					ApiRoot:   testAPIRoot,
+				}
+				return d.Configure(ctx, configureRequest)
+			},
+			want:    []string{},
+			wantErr: true,
+		},
+		{
+			name: "Configure with mock image",
+			docker: &Docker{
+				Image: mockImage,
+			},
+			runCommand: func(ctx context.Context, d *Docker) error {
+				configureRequest := &ConfigureRequest{
+					Cfg:       cfg,
+					State:     state,
+					LibraryID: testLibraryID,
+					RepoDir:   ".",
+					ApiRoot:   testAPIRoot,
+				}
+				return d.Configure(ctx, configureRequest)
+			},
+			want:    []string{},
+			wantErr: true,
+		},
 	} {
 		t.Run(test.name, func(t *testing.T) {
 			test.docker.run = func(args ...string) error {
diff --git a/internal/librarian/generate.go b/internal/librarian/generate.go
@@ -163,12 +163,16 @@ func (r *generateRunner) run(ctx context.Context) error {
 	}
 	slog.Info("Code will be generated", "dir", outputDir)
 
-	_, err := r.detectIfLibraryConfigured(ctx)
+	configured, err := r.detectIfLibraryConfigured(ctx)
 	if err != nil {
 		return err
 	}
 
-	// TODO(https://github.com/googleapis/librarian/issues/815)
+	if !configured {
+		if err := r.runConfigureCommand(ctx); err != nil {
+			return err
+		}
+	}
 
 	libraryID, err := r.runGenerateCommand(ctx, outputDir)
 	if err != nil {
@@ -416,6 +420,41 @@ func compileRegexps(patterns []string) ([]*regexp.Regexp, error) {
 	return regexps, nil
 }
 
+// runConfigureCommand executes the container's "configure" command for an API.
+//
+// This step prepares a language repository for a new library by adding
+// necessary metadata or build configurations. It finds the library's ID
+// from the runner's state, gathers all paths and settings, and then delegates
+// the execution to the container client.
+func (r *generateRunner) runConfigureCommand(ctx context.Context) error {
+	apiRoot, err := filepath.Abs(r.cfg.Source)
+	if err != nil {
+		return err
+	}
+
+	// Configuration requires a language repository to modify. If one isn't specified or
+	// found, we cannot proceed.
+	if r.repo == nil {
+		slog.Error("No language repository specified; cannot run configure.", "api", r.cfg.API)
+		return errors.New("a language repository must be specified to run configure")
+	}
+
+	libraryID := findLibraryIDByAPIPath(r.state, r.cfg.API)
+	if libraryID == "" {
+		return errors.New("bug in Librarian: Library not found during configuration, despite being found in earlier steps")
+	}
+
+	configureRequest := &docker.ConfigureRequest{
+		Cfg:       r.cfg,
+		State:     r.state,
+		ApiRoot:   apiRoot,
+		LibraryID: libraryID,
+		RepoDir:   r.repo.Dir,
+	}
+	slog.Info("Performing configuration for library", "id", libraryID)
+	return r.containerClient.Configure(ctx, configureRequest)
+}
+
 // detectIfLibraryConfigured returns whether a library has been configured for
 // the requested API (as specified in apiPath). This is done by checking the local
 // pipeline state if repoRoot has been specified, or the remote pipeline state (just
diff --git a/internal/librarian/generate_test.go b/internal/librarian/generate_test.go
@@ -24,18 +24,20 @@ import (
 	"strings"
 	"testing"
 
+	"github.com/googleapis/librarian/internal/docker"
+
 	"github.com/google/go-cmp/cmp"
 	"github.com/googleapis/librarian/internal/config"
-	"github.com/googleapis/librarian/internal/docker"
 	"github.com/googleapis/librarian/internal/gitrepo"
 	"gopkg.in/yaml.v3"
 )
 
 // mockContainerClient is a mock implementation of the ContainerClient interface for testing.
 type mockContainerClient struct {
 	ContainerClient
-	generateCalls int
-	buildCalls    int
+	generateCalls  int
+	buildCalls     int
+	configureCalls int
 }
 
 func (m *mockContainerClient) Generate(ctx context.Context, request *docker.GenerateRequest) error {
@@ -48,6 +50,11 @@ func (m *mockContainerClient) Build(ctx context.Context, request *docker.BuildRe
 	return nil
 }
 
+func (m *mockContainerClient) Configure(ctx context.Context, request *docker.ConfigureRequest) error {
+	m.configureCalls++
+	return nil
+}
+
 func TestDetectIfLibraryConfigured(t *testing.T) {
 	t.Parallel()
 	for _, test := range []struct {
@@ -287,6 +294,91 @@ func TestRunBuildCommand(t *testing.T) {
 	}
 }
 
+func TestRunConfigureCommand(t *testing.T) {
+	t.Parallel()
+	for _, test := range []struct {
+		name               string
+		api                string
+		source             string
+		repo               *gitrepo.Repository
+		state              *config.LibrarianState
+		container          *mockContainerClient
+		wantConfigureCalls int
+		wantErr            bool
+	}{
+		{
+			name: "configures library",
+			api:  "some/api",
+			repo: newTestGitRepo(t),
+			state: &config.LibrarianState{
+				Libraries: []*config.LibraryState{
+					{
+						ID:   "some-library",
+						APIs: []*config.API{{Path: "some/api"}},
+					},
+				},
+			},
+			container:          &mockContainerClient{},
+			wantConfigureCalls: 1,
+		},
+		{
+			name:      "missing repo",
+			api:       "some/api",
+			container: &mockContainerClient{},
+			wantErr:   true,
+		},
+		{
+			name: "library not found in state",
+			api:  "other/api",
+			repo: newTestGitRepo(t),
+			state: &config.LibrarianState{
+				Libraries: []*config.LibraryState{
+					{
+						ID:   "some-library",
+						APIs: []*config.API{{Path: "some/api"}},
+					},
+				},
+			},
+			container: &mockContainerClient{},
+			wantErr:   true,
+		},
+		{
+			name:               "error on invalid source path",
+			source:             "invalid path",
+			repo:               newTestGitRepo(t),
+			state:              &config.LibrarianState{},
+			container:          &mockContainerClient{},
+			wantConfigureCalls: 0,
+			wantErr:            true,
+		},
+	} {
+		t.Run(test.name, func(t *testing.T) {
+			t.Parallel()
+			sourcePath := test.source
+			if sourcePath == "" {
+				sourcePath = t.TempDir()
+			}
+			r := &generateRunner{
+				cfg: &config.Config{
+					API:    test.api,
+					Source: sourcePath,
+				},
+				repo:            test.repo,
+				state:           test.state,
+				containerClient: test.container,
+			}
+
+			if err := r.runConfigureCommand(context.Background()); (err != nil) != test.wantErr {
+				t.Errorf("runConfigureCommand() error = %v, wantErr %v", err, test.wantErr)
+				return
+			}
+			if diff := cmp.Diff(test.wantConfigureCalls, test.container.configureCalls); diff != "" {
+				t.Errorf("runConfigureCommand() configureCalls mismatch (-want +got):%s", diff)
+			}
+		})
+	}
+}
+
 func TestNewGenerateRunner(t *testing.T) {
 	t.Parallel()
 	for _, test := range []struct {
@@ -399,18 +491,19 @@ func runGit(t *testing.T, dir string, args ...string) {
 func TestGenerateRun(t *testing.T) {
 	t.Parallel()
 	for _, test := range []struct {
-		name              string
-		api               string
-		repo              *gitrepo.Repository
-		state             *config.LibrarianState
-		container         *mockContainerClient
-		build             bool
-		wantErr           bool
-		wantGenerateCalls int
-		wantBuildCalls    int
+		name               string
+		api                string
+		repo               *gitrepo.Repository
+		state              *config.LibrarianState
+		container          *mockContainerClient
+		build              bool
+		wantErr            bool
+		wantGenerateCalls  int
+		wantBuildCalls     int
+		wantConfigureCalls int
 	}{
 		{
-			name: "regeneration of API",
+			name: "generation of API",
 			api:  "some/api",
 			repo: newTestGitRepo(t),
 			state: &config.LibrarianState{
@@ -422,10 +515,11 @@ func TestGenerateRun(t *testing.T) {
 					},
 				},
 			},
-			container:         &mockContainerClient{},
-			build:             true,
-			wantGenerateCalls: 1,
-			wantBuildCalls:    1,
+			container:          &mockContainerClient{},
+			build:              true,
+			wantGenerateCalls:  1,
+			wantBuildCalls:     1,
+			wantConfigureCalls: 1,
 		},
 		{
 			name: "symlink in output",
@@ -487,6 +581,9 @@ func TestGenerateRun(t *testing.T) {
 			if diff := cmp.Diff(test.wantBuildCalls, test.container.buildCalls); diff != "" {
 				t.Errorf("run() buildCalls mismatch (-want +got):%s", diff)
 			}
+			if diff := cmp.Diff(test.wantConfigureCalls, test.container.configureCalls); diff != "" {
+				t.Errorf("run() configureCalls mismatch (-want +got):%s", diff)
+			}
 		})
 	}
 }
diff --git a/internal/librarian/librarian.go b/internal/librarian/librarian.go
