diff --git a/pkg/shaman/checkout/checkout_test.go b/pkg/shaman/checkout/checkout_test.go index c304b373..7f4358f4 100644 --- a/pkg/shaman/checkout/checkout_test.go +++ b/pkg/shaman/checkout/checkout_test.go @@ -3,7 +3,7 @@ package checkout import ( "context" "os" - "path" + "path/filepath" "testing" "git.blender.org/flamenco/pkg/api" @@ -34,21 +34,21 @@ func TestCheckout(t *testing.T) { } // Check the symlinks of the checkout - coPath := path.Join(manager.checkoutBasePath, actualCheckoutPath) - assert.FileExists(t, path.Join(coPath, "subdir", "replacer.py")) - assert.FileExists(t, path.Join(coPath, "feed.py")) - assert.FileExists(t, path.Join(coPath, "httpstuff.py")) - assert.FileExists(t, path.Join(coPath, "filesystemstuff.py")) + coPath := filepath.Join(manager.checkoutBasePath, actualCheckoutPath) + assert.FileExists(t, filepath.Join(coPath, "subdir", "replacer.py")) + assert.FileExists(t, filepath.Join(coPath, "feed.py")) + assert.FileExists(t, filepath.Join(coPath, "httpstuff.py")) + assert.FileExists(t, filepath.Join(coPath, "filesystemstuff.py")) storePath := manager.fileStore.StoragePath() - assertLinksTo(t, path.Join(coPath, "subdir", "replacer.py"), - path.Join(storePath, "59", "0c148428d5c35fab3ebad2f3365bb469ab9c531b60831f3e826c472027a0b9", "3367.blob")) - assertLinksTo(t, path.Join(coPath, "feed.py"), - path.Join(storePath, "80", "b749c27b2fef7255e7e7b3c2029b03b31299c75ff1f1c72732081c70a713a3", "7488.blob")) - assertLinksTo(t, path.Join(coPath, "httpstuff.py"), - path.Join(storePath, "91", "4853599dd2c351ab7b82b219aae6e527e51518a667f0ff32244b0c94c75688", "486.blob")) - assertLinksTo(t, path.Join(coPath, "filesystemstuff.py"), - path.Join(storePath, "d6", "fc7289b5196cc96748ea72f882a22c39b8833b457fe854ef4c03a01f5db0d3", "7217.blob")) + assertLinksTo(t, filepath.Join(coPath, "subdir", "replacer.py"), + filepath.Join(storePath, "59", "0c148428d5c35fab3ebad2f3365bb469ab9c531b60831f3e826c472027a0b9", "3367.blob")) + assertLinksTo(t, filepath.Join(coPath, "feed.py"), + filepath.Join(storePath, "80", "b749c27b2fef7255e7e7b3c2029b03b31299c75ff1f1c72732081c70a713a3", "7488.blob")) + assertLinksTo(t, filepath.Join(coPath, "httpstuff.py"), + filepath.Join(storePath, "91", "4853599dd2c351ab7b82b219aae6e527e51518a667f0ff32244b0c94c75688", "486.blob")) + assertLinksTo(t, filepath.Join(coPath, "filesystemstuff.py"), + filepath.Join(storePath, "d6", "fc7289b5196cc96748ea72f882a22c39b8833b457fe854ef4c03a01f5db0d3", "7217.blob")) } func assertLinksTo(t *testing.T, linkPath, expectedTarget string) { diff --git a/pkg/shaman/checkout/manager.go b/pkg/shaman/checkout/manager.go index 92c335a1..b425c53f 100644 --- a/pkg/shaman/checkout/manager.go +++ b/pkg/shaman/checkout/manager.go @@ -183,7 +183,7 @@ func (m *Manager) EraseCheckout(checkoutID string) error { // SymlinkToCheckout creates a symlink at symlinkPath to blobPath. // It does *not* do any validation of the validity of the paths! func (m *Manager) SymlinkToCheckout(blobPath, checkoutPath, symlinkRelativePath string) error { - symlinkPath := path.Join(checkoutPath, symlinkRelativePath) + symlinkPath := filepath.Join(checkoutPath, symlinkRelativePath) logger := log.With(). Str("blobPath", blobPath). Str("symlinkPath", symlinkPath). diff --git a/pkg/shaman/checkout/manager_test.go b/pkg/shaman/checkout/manager_test.go index ddb41b41..0839589a 100644 --- a/pkg/shaman/checkout/manager_test.go +++ b/pkg/shaman/checkout/manager_test.go @@ -25,7 +25,7 @@ package checkout import ( "io/ioutil" "os" - "path" + "path/filepath" "testing" "time" @@ -46,7 +46,7 @@ func TestSymlinkToCheckout(t *testing.T) { defer cleanup() // Fake an older file. - blobPath := path.Join(manager.checkoutBasePath, "jemoeder.blob") + blobPath := filepath.Join(manager.checkoutBasePath, "jemoeder.blob") err := ioutil.WriteFile(blobPath, []byte("op je hoofd"), 0600) assert.NoError(t, err) @@ -68,7 +68,7 @@ func TestSymlinkToCheckout(t *testing.T) { stat.ModTime().After(wayBackWhen), "File must be touched (%v must be later than %v)", stat.ModTime(), wayBackWhen) - symlinkPath := path.Join(manager.checkoutBasePath, symlinkRelativePath) + symlinkPath := filepath.Join(manager.checkoutBasePath, symlinkRelativePath) stat, err = os.Lstat(symlinkPath) assert.NoError(t, err) assert.True(t, stat.Mode()&os.ModeType == os.ModeSymlink, diff --git a/pkg/shaman/cleanup_test.go b/pkg/shaman/cleanup_test.go index a796a467..b219018f 100644 --- a/pkg/shaman/cleanup_test.go +++ b/pkg/shaman/cleanup_test.go @@ -25,6 +25,7 @@ package shaman import ( "os" "path" + "path/filepath" "testing" "time" @@ -43,7 +44,7 @@ func createTestShaman() (*Server, func()) { func makeOld(shaman *Server, expectOld mtimeMap, relPath string) { oldTime := time.Now().Add(-2 * shaman.config.GarbageCollect.MaxAge) - absPath := path.Join(shaman.config.FileStorePath(), relPath) + absPath := filepath.Join(shaman.config.FileStorePath(), relPath) err := os.Chtimes(absPath, oldTime, oldTime) if err != nil { @@ -95,7 +96,7 @@ func TestGCComponents(t *testing.T) { server, cleanup := createTestShaman() defer cleanup() - extraCheckoutDir := path.Join(server.config.TestTempDir, "extra-checkout") + extraCheckoutDir := filepath.Join(server.config.TestTempDir, "extra-checkout") server.config.GarbageCollect.ExtraCheckoutDirs = []string{extraCheckoutDir} filestore.LinkTestFileStore(server.config.FileStorePath()) @@ -131,10 +132,10 @@ func TestGCComponents(t *testing.T) { checkoutInfo, err := server.checkoutMan.PrepareCheckout("checkoutID") assert.NoError(t, err) err = server.checkoutMan.SymlinkToCheckout(absPaths["3367.blob"], server.config.CheckoutPath(), - path.Join(checkoutInfo.RelativePath, "use-of-3367.blob")) + filepath.Join(checkoutInfo.RelativePath, "use-of-3367.blob")) assert.NoError(t, err) err = server.checkoutMan.SymlinkToCheckout(absPaths["781.blob"], extraCheckoutDir, - path.Join(checkoutInfo.RelativePath, "use-of-781.blob")) + filepath.Join(checkoutInfo.RelativePath, "use-of-781.blob")) assert.NoError(t, err) // There should only be two old file reported now. @@ -179,7 +180,7 @@ func TestGarbageCollect(t *testing.T) { server, cleanup := createTestShaman() defer cleanup() - extraCheckoutDir := path.Join(server.config.TestTempDir, "extra-checkout") + extraCheckoutDir := filepath.Join(server.config.TestTempDir, "extra-checkout") server.config.GarbageCollect.ExtraCheckoutDirs = []string{extraCheckoutDir} filestore.LinkTestFileStore(server.config.FileStorePath()) @@ -201,10 +202,10 @@ func TestGarbageCollect(t *testing.T) { checkoutInfo, err := server.checkoutMan.PrepareCheckout("checkoutID") assert.NoError(t, err) err = server.checkoutMan.SymlinkToCheckout(absPaths["3367.blob"], server.config.CheckoutPath(), - path.Join(checkoutInfo.RelativePath, "use-of-3367.blob")) + filepath.Join(checkoutInfo.RelativePath, "use-of-3367.blob")) assert.NoError(t, err) err = server.checkoutMan.SymlinkToCheckout(absPaths["781.blob"], extraCheckoutDir, - path.Join(checkoutInfo.RelativePath, "use-of-781.blob")) + filepath.Join(checkoutInfo.RelativePath, "use-of-781.blob")) assert.NoError(t, err) // Running the garbage collector should only remove those two unused files. diff --git a/pkg/shaman/filestore/filestore.go b/pkg/shaman/filestore/filestore.go index 2a8f519d..e0e84b90 100644 --- a/pkg/shaman/filestore/filestore.go +++ b/pkg/shaman/filestore/filestore.go @@ -24,7 +24,7 @@ package filestore import ( "os" - "path" + "path/filepath" "strconv" "git.blender.org/flamenco/pkg/shaman/config" @@ -55,7 +55,7 @@ func New(conf config.Config) *Store { // Create the base directory structure for this store. func (s *Store) createDirectoryStructure() { mkdir := func(subdir string) { - path := path.Join(s.baseDir, subdir) + path := filepath.Join(s.baseDir, subdir) logger := log.With().Str("path", path).Logger() logger.Debug().Msg("shaman: creating directory") @@ -75,7 +75,7 @@ func (s *Store) createDirectoryStructure() { // StoragePath returns the directory path of the 'stored' storage bin. func (s *Store) StoragePath() string { - return path.Join(s.stored.basePath, s.stored.dirName) + return filepath.Join(s.stored.basePath, s.stored.dirName) } // BasePath returns the directory path of the storage. @@ -86,7 +86,7 @@ func (s *Store) BasePath() string { // Returns the checksum/filesize dependent parts of the file's path. // To be combined with a base directory, status directory, and status-dependent suffix. func (s *Store) partialFilePath(checksum string, filesize int64) string { - return path.Join(checksum[0:2], checksum[2:], strconv.FormatInt(filesize, 10)) + return filepath.Join(checksum[0:2], checksum[2:], strconv.FormatInt(filesize, 10)) } // ResolveFile checks the status of the file in the store. @@ -136,7 +136,7 @@ func (s *Store) MoveToStored(checksum string, filesize int64, uploadedFilePath s // Move to the other storage bin. targetPath := s.stored.pathFor(partial) - targetDir, _ := path.Split(targetPath) + targetDir, _ := filepath.Split(targetPath) if err := os.MkdirAll(targetDir, 0777); err != nil { return err } @@ -161,9 +161,9 @@ func (s *Store) removeFile(filePath string) error { } // Clean up directory structure, but ignore any errors (dirs may not be empty) - directory := path.Dir(filePath) + directory := filepath.Dir(filePath) os.Remove(directory) - os.Remove(path.Dir(directory)) + os.Remove(filepath.Dir(directory)) return err } @@ -177,7 +177,9 @@ func (s *Store) RemoveUploadedFile(filePath string) { Msg("shaman: RemoveUploadedFile called with file not in 'uploading' storage bin") return } - s.removeFile(filePath) + // Ignore the error here. It could very well be that the uploaded file has + // been moved somewhere else already. + _ = s.removeFile(filePath) } // RemoveStoredFile removes a file from the 'stored' storage bin. diff --git a/pkg/shaman/filestore/filestore_test.go b/pkg/shaman/filestore/filestore_test.go index 7d65b306..57d407e7 100644 --- a/pkg/shaman/filestore/filestore_test.go +++ b/pkg/shaman/filestore/filestore_test.go @@ -25,7 +25,7 @@ package filestore import ( "io/ioutil" "os" - "path" + "path/filepath" "testing" "github.com/stretchr/testify/assert" @@ -33,13 +33,13 @@ import ( // mustCreateFile creates an empty file. // The containing directory structure is created as well, if necessary. -func mustCreateFile(filepath string) { - err := os.MkdirAll(path.Dir(filepath), 0777) +func mustCreateFile(file_path string) { + err := os.MkdirAll(filepath.Dir(file_path), 0777) if err != nil { panic(err) } - file, err := os.Create(filepath) + file, err := os.Create(file_path) if err != nil { panic(err) } @@ -50,11 +50,11 @@ func TestCreateDirectories(t *testing.T) { store := CreateTestStore() defer CleanupTestStore(store) - assert.Equal(t, path.Join(store.baseDir, "uploading", "x"), store.uploading.storagePrefix("x")) - assert.Equal(t, path.Join(store.baseDir, "stored", "x"), store.stored.storagePrefix("x")) + assert.Equal(t, filepath.Join(store.baseDir, "uploading", "x"), store.uploading.storagePrefix("x")) + assert.Equal(t, filepath.Join(store.baseDir, "stored", "x"), store.stored.storagePrefix("x")) - assert.DirExists(t, path.Join(store.baseDir, "uploading")) - assert.DirExists(t, path.Join(store.baseDir, "stored")) + assert.DirExists(t, filepath.Join(store.baseDir, "uploading")) + assert.DirExists(t, filepath.Join(store.baseDir, "stored")) } func TestResolveStoredFile(t *testing.T) { @@ -65,7 +65,7 @@ func TestResolveStoredFile(t *testing.T) { assert.Equal(t, "", foundPath) assert.Equal(t, StatusDoesNotExist, status) - fname := path.Join(store.baseDir, "stored", "ab", "cdefxxx", "123.blob") + fname := filepath.Join(store.baseDir, "stored", "ab", "cdefxxx", "123.blob") mustCreateFile(fname) foundPath, status = store.ResolveFile("abcdefxxx", 123, ResolveStoredOnly) @@ -85,7 +85,7 @@ func TestResolveUploadingFile(t *testing.T) { assert.Equal(t, "", foundPath) assert.Equal(t, StatusDoesNotExist, status) - fname := path.Join(store.baseDir, "uploading", "ab", "cdefxxx", "123-unique-code.tmp") + fname := filepath.Join(store.baseDir, "uploading", "ab", "cdefxxx", "123-unique-code.tmp") mustCreateFile(fname) foundPath, status = store.ResolveFile("abcdefxxx", 123, ResolveStoredOnly) @@ -137,7 +137,7 @@ func TestMoveToStored(t *testing.T) { tempLocation := file.Name() err = store.MoveToStored("abcdefxxx", fileSize, file.Name()) - assert.NoError(t, err) + assert.NoError(t, err, "moving file %s", file.Name()) foundPath, status := store.ResolveFile("abcdefxxx", fileSize, ResolveEverything) assert.NotEqual(t, file.Name(), foundPath) @@ -146,12 +146,7 @@ func TestMoveToStored(t *testing.T) { assert.FileExists(t, foundPath) // The entire directory structure should be kept clean. - assertDoesNotExist(t, tempLocation) - assertDoesNotExist(t, path.Dir(tempLocation)) - assertDoesNotExist(t, path.Dir(path.Dir(tempLocation))) -} - -func assertDoesNotExist(t *testing.T, path string) { - _, err := os.Stat(path) - assert.True(t, os.IsNotExist(err), "%s should not exist, err=%v", path, err) + assert.NoFileExists(t, tempLocation) + assert.NoDirExists(t, filepath.Dir(tempLocation)) + assert.NoDirExists(t, filepath.Dir(filepath.Dir(tempLocation))) } diff --git a/pkg/shaman/filestore/substore.go b/pkg/shaman/filestore/substore.go index f47bafb2..b0da9474 100644 --- a/pkg/shaman/filestore/substore.go +++ b/pkg/shaman/filestore/substore.go @@ -25,7 +25,6 @@ package filestore import ( "errors" "os" - "path" "path/filepath" ) @@ -41,7 +40,7 @@ var ( ) func (s *storageBin) storagePrefix(partialPath string) string { - return path.Join(s.basePath, s.dirName, partialPath) + return filepath.Join(s.basePath, s.dirName, partialPath) } // Returns whether 'someFullPath' is pointing to a path inside our storage for the given partial path. @@ -95,7 +94,7 @@ func (s *storageBin) openForWriting(partialPath string) (*os.File, error) { } pathOrGlob := s.pathOrGlob(partialPath) - dirname, filename := path.Split(pathOrGlob) + dirname, filename := filepath.Split(pathOrGlob) if err := os.MkdirAll(dirname, 0777); err != nil { return nil, err diff --git a/pkg/shaman/filestore/substore_test.go b/pkg/shaman/filestore/substore_test.go index a971398c..806e9f93 100644 --- a/pkg/shaman/filestore/substore_test.go +++ b/pkg/shaman/filestore/substore_test.go @@ -24,6 +24,7 @@ package filestore import ( "os" + "path/filepath" "testing" "github.com/stretchr/testify/assert" @@ -34,10 +35,10 @@ func TestStoragePrefix(t *testing.T) { basePath: "/base", dirName: "testunit", } - assert.Equal(t, "/base/testunit", bin.storagePrefix("")) - assert.Equal(t, "/base/testunit", bin.storagePrefix("/")) - assert.Equal(t, "/base/testunit/xxx", bin.storagePrefix("xxx")) - assert.Equal(t, "/base/testunit/xxx", bin.storagePrefix("/xxx")) + assert.Equal(t, filepath.FromSlash("/base/testunit"), bin.storagePrefix("")) + assert.Equal(t, filepath.FromSlash("/base/testunit"), bin.storagePrefix("/")) + assert.Equal(t, filepath.FromSlash("/base/testunit/xxx"), bin.storagePrefix("xxx")) + assert.Equal(t, filepath.FromSlash("/base/testunit/xxx"), bin.storagePrefix("/xxx")) } func TestContains(t *testing.T) { @@ -45,9 +46,9 @@ func TestContains(t *testing.T) { basePath: "/base", dirName: "testunit", } - assert.True(t, bin.contains("", "/base/testunit/jemoeder.txt")) - assert.True(t, bin.contains("jemoeder", "/base/testunit/jemoeder.txt")) - assert.False(t, bin.contains("jemoeder", "/base/testunit/opjehoofd/jemoeder.txt")) + assert.True(t, bin.contains("", filepath.FromSlash("/base/testunit/jemoeder.txt"))) + assert.True(t, bin.contains("jemoeder", filepath.FromSlash("/base/testunit/jemoeder.txt"))) + assert.False(t, bin.contains("jemoeder", filepath.FromSlash("/base/testunit/opjehoofd/jemoeder.txt"))) assert.False(t, bin.contains("", "/etc/passwd")) assert.False(t, bin.contains("/", "/etc/passwd")) assert.False(t, bin.contains("/etc", "/etc/passwd")) diff --git a/pkg/shaman/filestore/testing.go b/pkg/shaman/filestore/testing.go index d663f9c2..d525d9e8 100644 --- a/pkg/shaman/filestore/testing.go +++ b/pkg/shaman/filestore/testing.go @@ -80,7 +80,7 @@ func (s *Store) MustStoreFileForTest(checksum string, filesize int64, contents [ // Panics if there are any errors. func LinkTestFileStore(cloneTo string) { _, myFilename, _, _ := runtime.Caller(0) - fileStorePath := path.Join(path.Dir(path.Dir(myFilename)), "_test_file_store") + fileStorePath := filepath.Join(path.Dir(path.Dir(myFilename)), "_test_file_store") now := time.Now() visit := func(visitPath string, info os.FileInfo, err error) error { @@ -93,7 +93,7 @@ func LinkTestFileStore(cloneTo string) { return err } - targetPath := path.Join(cloneTo, relpath) + targetPath := filepath.Join(cloneTo, relpath) if info.IsDir() { return os.MkdirAll(targetPath, 0755) }