fix(internal/librarian): don't clean dir for release init copies (#1824)

codyoss · web-flow · commit a8ddd21b41f3 · 2025-08-26T12:00:56.000-05:00
We should intentionally copy over only the files that are in the output directory of the release init container. We don't mandate today that all libraries files must be copied over, so we should selectively overwrite files when required. Fixes: #1806
diff --git a/internal/librarian/command.go b/internal/librarian/command.go
@@ -208,6 +208,60 @@ func cleanAndCopyLibrary(state *config.LibrarianState, repoDir, libraryID, outpu
 	return copyLibrary(repoDir, outputDir, library)
 }
 
+func copyLibraryFiles(state *config.LibrarianState, dest, libraryID, src string) error {
+	library := findLibraryByID(state, libraryID)
+	if library == nil {
+		return fmt.Errorf("library %q not found", libraryID)
+	}
+	slog.Info("Copying library files", "id", library.ID, "destination", dest, "source", src)
+	for _, srcRoot := range library.SourceRoots {
+		dstPath := filepath.Join(dest, srcRoot)
+		srcPath := filepath.Join(src, srcRoot)
+		files, err := getDirectoryFilesnames(srcPath)
+		if err != nil {
+			return err
+		}
+		for _, file := range files {
+			slog.Info("Copying file", "file", file)
+			srcFile := filepath.Join(srcPath, file)
+			dstFile := filepath.Join(dstPath, file)
+			if err := copyFile(dstFile, srcFile); err != nil {
+				return fmt.Errorf("failed to copy file %q for library %s: %w", srcFile, library.ID, err)
+			}
+		}
+	}
+	return nil
+}
+
+func getDirectoryFilesnames(dir string) ([]string, error) {
+	if _, err := os.Stat(dir); err != nil {
+		// Skip dirs that don't exist
+		if os.IsNotExist(err) {
+			return nil, nil
+		}
+		return nil, err
+	}
+
+	var fileNames []string
+	err := filepath.WalkDir(dir, func(path string, d os.DirEntry, err error) error {
+		if err != nil {
+			return err
+		}
+		if !d.IsDir() {
+			relativePath, err := filepath.Rel(dir, path)
+			if err != nil {
+				return err
+			}
+			fileNames = append(fileNames, relativePath)
+		}
+		return nil
+	})
+	if err != nil {
+		return nil, err
+	}
+	return fileNames, nil
+}
+
 // copyLibrary copies library file from src to dst.
 func copyLibrary(dst, src string, library *config.LibraryState) error {
 	slog.Info("Copying library", "id", library.ID, "destination", dst, "source", src)
diff --git a/internal/librarian/command_test.go b/internal/librarian/command_test.go
@@ -725,6 +725,141 @@ func TestCommitAndPush(t *testing.T) {
 	}
 }
 
+func TestCopyLibraryFiles(t *testing.T) {
+	t.Parallel()
+	setup := func(src string, files []string) {
+		for _, relPath := range files {
+			fullPath := filepath.Join(src, relPath)
+			if err := os.MkdirAll(filepath.Dir(fullPath), 0755); err != nil {
+				t.Error(err)
+			}
+
+			if _, err := os.Create(fullPath); err != nil {
+				t.Error(err)
+			}
+		}
+	}
+	for _, test := range []struct {
+		name          string
+		repoDir       string
+		outputDir     string
+		libraryID     string
+		state         *config.LibrarianState
+		filesToCreate []string
+		wantFiles     []string
+		skipFiles     []string
+		wantErr       bool
+		wantErrMsg    string
+	}{
+		{
+			name:      "copy library files",
+			repoDir:   filepath.Join(t.TempDir(), "dst"),
+			outputDir: filepath.Join(t.TempDir(), "src"),
+			libraryID: "example-library",
+			state: &config.LibrarianState{
+				Libraries: []*config.LibraryState{
+					{
+						ID: "example-library",
+						SourceRoots: []string{
+							"a/path",
+							"another/path",
+						},
+					},
+				},
+			},
+			filesToCreate: []string{
+				"a/path/example.txt",
+				"another/path/example.txt",
+				"skipped/path/example.txt",
+			},
+			wantFiles: []string{
+				"a/path/example.txt",
+				"another/path/example.txt",
+			},
+			skipFiles: []string{
+				"skipped/path/example.txt",
+			},
+		},
+		{
+			name:      "library not found",
+			repoDir:   filepath.Join(t.TempDir(), "dst"),
+			outputDir: filepath.Join(t.TempDir(), "src"),
+			libraryID: "non-existent-library",
+			state: &config.LibrarianState{
+				Libraries: []*config.LibraryState{
+					{
+						ID: "example-library",
+					},
+				},
+			},
+			wantErr:    true,
+			wantErrMsg: "not found",
+		},
+		{
+			repoDir:   filepath.Join(t.TempDir(), "dst"),
+			name:      "one source root empty",
+			outputDir: filepath.Join(t.TempDir(), "src"),
+			libraryID: "example-library",
+			state: &config.LibrarianState{
+				Libraries: []*config.LibraryState{
+					{
+						ID: "example-library",
+						SourceRoots: []string{
+							"a/path",
+							"another/path",
+						},
+					},
+				},
+			},
+			filesToCreate: []string{
+				"a/path/example.txt",
+				"skipped/path/example.txt",
+			},
+			wantFiles: []string{
+				"a/path/example.txt",
+			},
+			skipFiles: []string{
+				"skipped/path/example.txt",
+			},
+		},
+	} {
+		t.Run(test.name, func(t *testing.T) {
+			if !test.wantErr {
+				setup(test.outputDir, test.filesToCreate)
+			}
+			err := copyLibraryFiles(test.state, test.repoDir, test.libraryID, test.outputDir)
+			if test.wantErr {
+				if err == nil {
+					t.Errorf("copyLibraryFiles() shoud fail")
+				}
+
+				if !strings.Contains(err.Error(), test.wantErrMsg) {
+					t.Errorf("want error message: %s, got: %s", test.wantErrMsg, err.Error())
+				}
+
+				return
+			}
+			if err != nil {
+				t.Errorf("failed to run copyLibraryFiles(): %s", err.Error())
+			}
+
+			for _, file := range test.wantFiles {
+				fullPath := filepath.Join(test.repoDir, file)
+				if _, err := os.Stat(fullPath); err != nil {
+					t.Errorf("file %s is not copied to %s", file, test.repoDir)
+				}
+			}
+
+			for _, file := range test.skipFiles {
+				fullPath := filepath.Join(test.repoDir, file)
+				if _, err := os.Stat(fullPath); !os.IsNotExist(err) {
+					t.Errorf("file %s should not be copied to %s", file, test.repoDir)
+				}
+			}
+		})
+	}
+}
+
 func TestCopyFile(t *testing.T) {
 	t.Parallel()
 	for _, test := range []struct {
diff --git a/internal/librarian/release_init.go b/internal/librarian/release_init.go
@@ -152,7 +152,7 @@ func (r *initRunner) runInitCommand(ctx context.Context, outputDir string) error
 		return fmt.Errorf("failed to copy librarian dir from %s to %s: %w", src, dst, err)
 	}
 
-	if err := cleanAndCopyGlobalAllowlist(r.librarianConfig, dst, src); err != nil {
+	if err := copyGlobalAllowlist(r.librarianConfig, dst, src, true); err != nil {
 		return fmt.Errorf("failed to copy global allowlist  from %s to %s: %w", src, dst, err)
 	}
 
@@ -176,20 +176,20 @@ func (r *initRunner) runInitCommand(ctx context.Context, outputDir string) error
 				continue
 			}
 			// Only copy one library to repository.
-			if err := cleanAndCopyLibrary(r.state, r.repo.GetDir(), r.cfg.Library, outputDir); err != nil {
+			if err := copyLibraryFiles(r.state, r.repo.GetDir(), r.cfg.Library, outputDir); err != nil {
 				return err
 			}
 
 			break
 		}
 
 		// Copy all libraries to repository.
-		if err := cleanAndCopyLibrary(r.state, r.repo.GetDir(), library.ID, outputDir); err != nil {
+		if err := copyLibraryFiles(r.state, r.repo.GetDir(), library.ID, outputDir); err != nil {
 			return err
 		}
 	}
 
-	return cleanAndCopyGlobalAllowlist(r.librarianConfig, r.repo.GetDir(), outputDir)
+	return copyGlobalAllowlist(r.librarianConfig, r.repo.GetDir(), outputDir, false)
 }
 
 // updateLibrary updates the given library in the following way:
@@ -253,29 +253,26 @@ func getChangeType(commit *conventionalcommits.ConventionalCommit) string {
 	return changeType
 }
 
-// cleanAndCopyGlobalAllowlist cleans the files listed in global allowlist in
-// src, excluding read-only files and copies global files from src.
-func cleanAndCopyGlobalAllowlist(cfg *config.LibrarianConfig, dst, src string) error {
+// copyGlobalAllowlist copies files in the global file allowlist excluding
+//
+//	read-only files and copies global files from src.
+func copyGlobalAllowlist(cfg *config.LibrarianConfig, dst, src string, copyReadOnly bool) error {
 	if cfg == nil {
 		slog.Info("librarian config is not setup, skip copying global allowlist")
 		return nil
 	}
+	slog.Info("Copying global allowlist files", "destination", dst, "source", src)
 	for _, globalFile := range cfg.GlobalFilesAllowlist {
-		if globalFile.Permissions == config.PermissionReadOnly {
+		if globalFile.Permissions == config.PermissionReadOnly && !copyReadOnly {
+			slog.Debug("skipping read-only file", "path", globalFile.Path)
 			continue
 		}
-
-		dstPath := filepath.Join(dst, globalFile.Path)
-		if err := os.Remove(dstPath); err != nil {
-			return fmt.Errorf("failed to remove global file %s: %w", dstPath, err)
-		}
-
 		srcPath := filepath.Join(src, globalFile.Path)
+		dstPath := filepath.Join(dst, globalFile.Path)
 		if err := copyFile(dstPath, srcPath); err != nil {
 			return fmt.Errorf("failed to copy global file %s from %s: %w", dstPath, srcPath, err)
 		}
 	}
-
 	return nil
 }
 
diff --git a/internal/librarian/release_init_test.go b/internal/librarian/release_init_test.go
@@ -549,7 +549,7 @@ func TestUpdateLibrary(t *testing.T) {
 	}
 }
 
-func TestCleanAndCopyGlobalAllowlist(t *testing.T) {
+func TestCopyGlobalAllowlist(t *testing.T) {
 	t.Parallel()
 	for _, test := range []struct {
 		name              string
@@ -560,6 +560,7 @@ func TestCleanAndCopyGlobalAllowlist(t *testing.T) {
 		doNotCreateOutput bool // do not create files in output dir.
 		wantErr           bool
 		wantErrMsg        string
+		copyReadOnly      bool
 	}{
 		{
 			name: "copied all global allowlist",
@@ -634,7 +635,7 @@ func TestCleanAndCopyGlobalAllowlist(t *testing.T) {
 				"ignored/path/example.txt",
 			},
 			wantErr:    true,
-			wantErrMsg: "failed to remove global file",
+			wantErrMsg: "failed to open file",
 		},
 		{
 			name: "output doesn't have the global file",
@@ -653,6 +654,34 @@ func TestCleanAndCopyGlobalAllowlist(t *testing.T) {
 			wantErr:           true,
 			wantErrMsg:        "failed to copy global file",
 		},
+		{
+			name:         "copies read-only files",
+			copyReadOnly: true,
+			cfg: &config.LibrarianConfig{
+				GlobalFilesAllowlist: []*config.GlobalFile{
+					{
+						Path:        "one/path/example.txt",
+						Permissions: "read-write",
+					},
+					{
+						Path:        "another/path/example.txt",
+						Permissions: "read-only",
+					},
+				},
+			},
+			files: []string{
+				"one/path/example.txt",
+				"another/path/example.txt",
+				"ignored/path/example.txt",
+			},
+			copied: []string{
+				"one/path/example.txt",
+				"another/path/example.txt",
+			},
+			skipped: []string{
+				"ignored/path/example.txt",
+			},
+		},
 	} {
 		t.Run(test.name, func(t *testing.T) {
 			output := t.TempDir()
@@ -687,7 +716,7 @@ func TestCleanAndCopyGlobalAllowlist(t *testing.T) {
 				}
 			}
 
-			err := cleanAndCopyGlobalAllowlist(test.cfg, repo, output)
+			err := copyGlobalAllowlist(test.cfg, repo, output, test.copyReadOnly)
 
 			if test.wantErr {
 				if err == nil {

Original file line number	Diff line number	Diff line change
`@@ -152,7 +152,7 @@ func (r *initRunner) runInitCommand(ctx context.Context, outputDir string) error`
`152`	`152`	`return fmt.Errorf("failed to copy librarian dir from %s to %s: %w", src, dst, err)`
`153`	`153`	`}`
`154`	`154`
`155`		`- if err := cleanAndCopyGlobalAllowlist(r.librarianConfig, dst, src); err != nil {`
	`155`	`+ if err := copyGlobalAllowlist(r.librarianConfig, dst, src, true); err != nil {`
`156`	`156`	`return fmt.Errorf("failed to copy global allowlist from %s to %s: %w", src, dst, err)`
`157`	`157`	`}`
`158`	`158`
`@@ -176,20 +176,20 @@ func (r *initRunner) runInitCommand(ctx context.Context, outputDir string) error`
`176`	`176`	`continue`
`177`	`177`	`}`
`178`	`178`	`// Only copy one library to repository.`
`179`		`- if err := cleanAndCopyLibrary(r.state, r.repo.GetDir(), r.cfg.Library, outputDir); err != nil {`
	`179`	`+ if err := copyLibraryFiles(r.state, r.repo.GetDir(), r.cfg.Library, outputDir); err != nil {`
`180`	`180`	`return err`
`181`	`181`	`}`
`182`	`182`
`183`	`183`	`break`
`184`	`184`	`}`
`185`	`185`
`186`	`186`	`// Copy all libraries to repository.`
`187`		`- if err := cleanAndCopyLibrary(r.state, r.repo.GetDir(), library.ID, outputDir); err != nil {`
	`187`	`+ if err := copyLibraryFiles(r.state, r.repo.GetDir(), library.ID, outputDir); err != nil {`
`188`	`188`	`return err`
`189`	`189`	`}`
`190`	`190`	`}`
`191`	`191`
`192`		`- return cleanAndCopyGlobalAllowlist(r.librarianConfig, r.repo.GetDir(), outputDir)`
	`192`	`+ return copyGlobalAllowlist(r.librarianConfig, r.repo.GetDir(), outputDir, false)`
`193`	`193`	`}`
`194`	`194`
`195`	`195`	`// updateLibrary updates the given library in the following way:`
`@@ -253,29 +253,26 @@ func getChangeType(commit *conventionalcommits.ConventionalCommit) string {`
`253`	`253`	`return changeType`
`254`	`254`	`}`
`255`	`255`
`256`		`-// cleanAndCopyGlobalAllowlist cleans the files listed in global allowlist in`
`257`		`-// src, excluding read-only files and copies global files from src.`
`258`		`-func cleanAndCopyGlobalAllowlist(cfg *config.LibrarianConfig, dst, src string) error {`
	`256`	`+// copyGlobalAllowlist copies files in the global file allowlist excluding`
	`257`	`+//`
	`258`	`+// read-only files and copies global files from src.`
	`259`	`+func copyGlobalAllowlist(cfg *config.LibrarianConfig, dst, src string, copyReadOnly bool) error {`
`259`	`260`	`if cfg == nil {`
`260`	`261`	`slog.Info("librarian config is not setup, skip copying global allowlist")`
`261`	`262`	`return nil`
`262`	`263`	`}`
	`264`	`+ slog.Info("Copying global allowlist files", "destination", dst, "source", src)`
`263`	`265`	`for _, globalFile := range cfg.GlobalFilesAllowlist {`
`264`		`- if globalFile.Permissions == config.PermissionReadOnly {`
	`266`	`+ if globalFile.Permissions == config.PermissionReadOnly && !copyReadOnly {`
	`267`	`+ slog.Debug("skipping read-only file", "path", globalFile.Path)`
`265`	`268`	`continue`
`266`	`269`	`}`
`267`		`-`
`268`		`- dstPath := filepath.Join(dst, globalFile.Path)`
`269`		`- if err := os.Remove(dstPath); err != nil {`
`270`		`- return fmt.Errorf("failed to remove global file %s: %w", dstPath, err)`
`271`		`- }`
`272`		`-`
`273`	`270`	`srcPath := filepath.Join(src, globalFile.Path)`
	`271`	`+ dstPath := filepath.Join(dst, globalFile.Path)`
`274`	`272`	`if err := copyFile(dstPath, srcPath); err != nil {`
`275`	`273`	`return fmt.Errorf("failed to copy global file %s from %s: %w", dstPath, srcPath, err)`
`276`	`274`	`}`
`277`	`275`	`}`
`278`		`-`
`279`	`276`	`return nil`
`280`	`277`	`}`
`281`	`278`