From 9e7aaa3f2610321e94b74a8ef9abc8ccd33e9954 Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Mon, 18 Nov 2024 21:27:01 +0100 Subject: [PATCH 1/4] Make 'persistToDisk' take a context argument --- libs/template/file.go | 6 +++--- libs/template/file_test.go | 20 ++++++++++++-------- libs/template/materialize.go | 2 +- libs/template/renderer.go | 4 ++-- libs/template/renderer_test.go | 17 +++++++++-------- 5 files changed, 27 insertions(+), 22 deletions(-) diff --git a/libs/template/file.go b/libs/template/file.go index 5492ebeb4d..7377334c71 100644 --- a/libs/template/file.go +++ b/libs/template/file.go @@ -17,7 +17,7 @@ type file interface { DstPath() *destinationPath // Write file to disk at the destination path. - PersistToDisk() error + Write(ctx context.Context) error // contents returns the file contents as a byte slice. // This is used for testing purposes. @@ -60,7 +60,7 @@ func (f *copyFile) DstPath() *destinationPath { return f.dstPath } -func (f *copyFile) PersistToDisk() error { +func (f *copyFile) Write(ctx context.Context) error { path := f.DstPath().absPath() err := os.MkdirAll(filepath.Dir(path), 0755) if err != nil { @@ -97,7 +97,7 @@ func (f *inMemoryFile) DstPath() *destinationPath { return f.dstPath } -func (f *inMemoryFile) PersistToDisk() error { +func (f *inMemoryFile) Write(ctx context.Context) error { path := f.DstPath().absPath() err := os.MkdirAll(filepath.Dir(path), 0755) diff --git a/libs/template/file_test.go b/libs/template/file_test.go index e1bd54564c..a75a23565b 100644 --- a/libs/template/file_test.go +++ b/libs/template/file_test.go @@ -12,7 +12,7 @@ import ( "github.com/stretchr/testify/require" ) -func testInMemoryFile(t *testing.T, perm fs.FileMode) { +func testInMemoryFile(t *testing.T, ctx context.Context, perm fs.FileMode) { tmpDir := t.TempDir() f := &inMemoryFile{ @@ -23,14 +23,14 @@ func testInMemoryFile(t *testing.T, perm fs.FileMode) { perm: perm, content: []byte("123"), } - err := f.PersistToDisk() + err := f.Write(ctx) assert.NoError(t, err) assertFileContent(t, filepath.Join(tmpDir, "a/b/c"), "123") assertFilePermissions(t, filepath.Join(tmpDir, "a/b/c"), perm) } -func testCopyFile(t *testing.T, perm fs.FileMode) { +func testCopyFile(t *testing.T, ctx context.Context, perm fs.FileMode) { tmpDir := t.TempDir() err := os.WriteFile(filepath.Join(tmpDir, "source"), []byte("qwerty"), perm) require.NoError(t, err) @@ -45,7 +45,7 @@ func testCopyFile(t *testing.T, perm fs.FileMode) { srcPath: "source", srcFS: os.DirFS(tmpDir), } - err = f.PersistToDisk() + err = f.Write(ctx) assert.NoError(t, err) assertFileContent(t, filepath.Join(tmpDir, "a/b/c"), "qwerty") @@ -78,7 +78,8 @@ func TestTemplateInMemoryFilePersistToDisk(t *testing.T) { if runtime.GOOS == "windows" { t.SkipNow() } - testInMemoryFile(t, 0755) + ctx := context.Background() + testInMemoryFile(t, ctx, 0755) } func TestTemplateInMemoryFilePersistToDiskForWindows(t *testing.T) { @@ -87,14 +88,16 @@ func TestTemplateInMemoryFilePersistToDiskForWindows(t *testing.T) { } // we have separate tests for windows because of differences in valid // fs.FileMode values we can use for different operating systems. - testInMemoryFile(t, 0666) + ctx := context.Background() + testInMemoryFile(t, ctx, 0666) } func TestTemplateCopyFilePersistToDisk(t *testing.T) { if runtime.GOOS == "windows" { t.SkipNow() } - testCopyFile(t, 0644) + ctx := context.Background() + testCopyFile(t, ctx, 0644) } func TestTemplateCopyFilePersistToDiskForWindows(t *testing.T) { @@ -103,5 +106,6 @@ func TestTemplateCopyFilePersistToDiskForWindows(t *testing.T) { } // we have separate tests for windows because of differences in valid // fs.FileMode values we can use for different operating systems. - testCopyFile(t, 0666) + ctx := context.Background() + testCopyFile(t, ctx, 0666) } diff --git a/libs/template/materialize.go b/libs/template/materialize.go index 0163eb7d22..502789162b 100644 --- a/libs/template/materialize.go +++ b/libs/template/materialize.go @@ -72,7 +72,7 @@ func Materialize(ctx context.Context, configFilePath string, templateFS fs.FS, o return err } - err = r.persistToDisk() + err = r.persistToDisk(ctx) if err != nil { return err } diff --git a/libs/template/renderer.go b/libs/template/renderer.go index bc86503993..1f36d27b38 100644 --- a/libs/template/renderer.go +++ b/libs/template/renderer.go @@ -299,7 +299,7 @@ func (r *renderer) walk() error { return nil } -func (r *renderer) persistToDisk() error { +func (r *renderer) persistToDisk(ctx context.Context) error { // Accumulate files which we will persist, skipping files whose path matches // any of the skip patterns filesToPersist := make([]file, 0) @@ -329,7 +329,7 @@ func (r *renderer) persistToDisk() error { // Persist files to disk for _, file := range filesToPersist { - err := file.PersistToDisk() + err := file.Write(ctx) if err != nil { return err } diff --git a/libs/template/renderer_test.go b/libs/template/renderer_test.go index 9b8861e786..bc6167b313 100644 --- a/libs/template/renderer_test.go +++ b/libs/template/renderer_test.go @@ -63,7 +63,7 @@ func assertBuiltinTemplateValid(t *testing.T, template string, settings map[stri // Evaluate template err = renderer.walk() require.NoError(t, err) - err = renderer.persistToDisk() + err = renderer.persistToDisk(ctx) require.NoError(t, err) b, err := bundle.Load(ctx, filepath.Join(tempDir, "my_project")) @@ -187,7 +187,7 @@ func TestRendererWithAssociatedTemplateInLibrary(t *testing.T) { err = r.walk() require.NoError(t, err) - err = r.persistToDisk() + err = r.persistToDisk(ctx) require.NoError(t, err) b, err := os.ReadFile(filepath.Join(tmpDir, "my_email")) @@ -350,7 +350,7 @@ func TestRendererPersistToDisk(t *testing.T) { }, } - err := r.persistToDisk() + err := r.persistToDisk(ctx) require.NoError(t, err) assert.NoFileExists(t, filepath.Join(tmpDir, "a", "b", "c")) @@ -438,7 +438,7 @@ func TestRendererSkipAllFilesInCurrentDirectory(t *testing.T) { // All 3 files are executed and have in memory representations require.Len(t, r.files, 3) - err = r.persistToDisk() + err = r.persistToDisk(ctx) require.NoError(t, err) entries, err := os.ReadDir(tmpDir) @@ -480,7 +480,7 @@ func TestRendererSkip(t *testing.T) { // This is because "dir2/*" matches the files in dir2, but not dir2 itself assert.Len(t, r.files, 6) - err = r.persistToDisk() + err = r.persistToDisk(ctx) require.NoError(t, err) assert.FileExists(t, filepath.Join(tmpDir, "file1")) @@ -534,6 +534,7 @@ func TestRendererReadsPermissionsBits(t *testing.T) { func TestRendererErrorOnConflictingFile(t *testing.T) { tmpDir := t.TempDir() + ctx := context.Background() f, err := os.Create(filepath.Join(tmpDir, "a")) require.NoError(t, err) @@ -553,7 +554,7 @@ func TestRendererErrorOnConflictingFile(t *testing.T) { }, }, } - err = r.persistToDisk() + err = r.persistToDisk(ctx) assert.EqualError(t, err, fmt.Sprintf("failed to initialize template, one or more files already exist: %s", filepath.Join(tmpDir, "a"))) } @@ -580,7 +581,7 @@ func TestRendererNoErrorOnConflictingFileIfSkipped(t *testing.T) { }, }, } - err = r.persistToDisk() + err = r.persistToDisk(ctx) // No error is returned even though a conflicting file exists. This is because // the generated file is being skipped assert.NoError(t, err) @@ -623,7 +624,7 @@ func TestRendererFileTreeRendering(t *testing.T) { assert.Len(t, r.files, 1) assert.Equal(t, r.files[0].DstPath().absPath(), filepath.Join(tmpDir, "my_directory", "my_file")) - err = r.persistToDisk() + err = r.persistToDisk(ctx) require.NoError(t, err) // Assert files and directories are correctly materialized. From 092bc469761af14a9914b6650d6d0095373ba644 Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Mon, 18 Nov 2024 22:28:28 +0100 Subject: [PATCH 2/4] Use filer for materializing template files --- libs/filer/filer.go | 11 +++- libs/filer/filer_test.go | 11 ++++ libs/filer/local_client.go | 13 +++- libs/template/config_test.go | 4 +- libs/template/file.go | 84 ++++++++----------------- libs/template/file_test.go | 46 ++++---------- libs/template/helpers_test.go | 24 +++----- libs/template/materialize.go | 10 ++- libs/template/renderer.go | 32 +++------- libs/template/renderer_test.go | 109 +++++++++++++++------------------ 10 files changed, 147 insertions(+), 197 deletions(-) create mode 100644 libs/filer/filer_test.go diff --git a/libs/filer/filer.go b/libs/filer/filer.go index fcfbcea071..a748e1fd45 100644 --- a/libs/filer/filer.go +++ b/libs/filer/filer.go @@ -7,13 +7,22 @@ import ( "io/fs" ) +// WriteMode captures intent when writing a file. +// +// The first 9 bits are reserved for the [fs.FileMode] permission bits. +// These are used only by the local filer implementation and have +// no effect for the other implementations. type WriteMode int +// writeModePerm is a mask to extract permission bits from a WriteMode. +const writeModePerm = WriteMode(fs.ModePerm) + const ( - OverwriteIfExists WriteMode = 1 << iota + OverwriteIfExists WriteMode = writeModePerm + 1< Date: Tue, 19 Nov 2024 16:54:11 +0100 Subject: [PATCH 3/4] Maintain mask invariant --- libs/filer/filer.go | 2 +- libs/filer/filer_test.go | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/libs/filer/filer.go b/libs/filer/filer.go index a748e1fd45..4d6facbe5d 100644 --- a/libs/filer/filer.go +++ b/libs/filer/filer.go @@ -18,7 +18,7 @@ type WriteMode int const writeModePerm = WriteMode(fs.ModePerm) const ( - OverwriteIfExists WriteMode = writeModePerm + 1< Date: Wed, 20 Nov 2024 10:31:19 +0100 Subject: [PATCH 4/4] Add comment to describe WriteMode mask starting point --- libs/filer/filer.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/libs/filer/filer.go b/libs/filer/filer.go index 4d6facbe5d..b5be4c3c2f 100644 --- a/libs/filer/filer.go +++ b/libs/filer/filer.go @@ -18,6 +18,8 @@ type WriteMode int const writeModePerm = WriteMode(fs.ModePerm) const ( + // Note: these constants are defined as powers of 2 to support combining them using a bit-wise OR. + // They starts from the 10th bit (permission mask + 1) to avoid conflicts with the permission bits. OverwriteIfExists WriteMode = (writeModePerm + 1) << iota CreateParentDirectories )