From e34cd160e2682ae141771b93beef0ee26cb438c6 Mon Sep 17 00:00:00 2001 From: Marcello DeSales Date: Tue, 28 Jan 2025 15:23:53 -0800 Subject: [PATCH 1/4] :hamster: :bug: fix cachedirectory.go to reuse empty dir For the cases of cache directories that need to exist before, the code fails with the error message that it can't create the directory. For example, cache dir from github workflows. This code change makes sure to verify the cases if it's empty or not. --- internal/cachedirectory/cachedirectory.go | 56 +++++++++++++++++------ 1 file changed, 43 insertions(+), 13 deletions(-) diff --git a/internal/cachedirectory/cachedirectory.go b/internal/cachedirectory/cachedirectory.go index 6838e12..3644ac5 100644 --- a/internal/cachedirectory/cachedirectory.go +++ b/internal/cachedirectory/cachedirectory.go @@ -27,24 +27,37 @@ func NewCacheDirectory(path string) CacheDirectory { } } -func isEmptyOrNonExistentDirectory(path string) (bool, error) { - f, err := os.Open(path) +func isAccessibleDirectory(path string) (bool, error) { + _, err := os.Stat(path) + if err != nil { if os.IsNotExist(err) { - return true, nil + return false, nil } return false, errors.Wrapf(err, "Could not access directory %s.", path) } - defer f.Close() + + return true, nil +} - _, err = f.Readdirnames(1) +func isEmptyDirectory(path string) (bool, error) { + files, err := ioutil.ReadDir(path) if err != nil { - if err == io.EOF { - return true, nil - } return false, errors.Wrapf(err, "Could not read contents of directory %s.", path) } - return false, nil + + return len(files) == 0, nil +} + +func existsDirectory(path string) (bool, error) { + _, err := os.Stat(path) + if os.IsNotExist(err) { + return false, nil + } + if err != nil { + return false, errors.Wrapf(err, "Could not access directory %s.", path) + } + return true, nil } func (cacheDirectory *CacheDirectory) CheckOrCreateVersionFile(pull bool, version string) error { @@ -77,20 +90,37 @@ func (cacheDirectory *CacheDirectory) CheckOrCreateVersionFile(pull bool, versio } } - isEmptyOrNonExistent, err := isEmptyOrNonExistentDirectory(cacheDirectory.path) + existsDirectory, err := existsDirectory(cacheDirectory.path) if err != nil { return err } - if isEmptyOrNonExistent { + if !existsDirectory { err := os.Mkdir(cacheDirectory.path, 0755) if err != nil { return errors.Wrap(err, "Could not create cache directory.") } - err = ioutil.WriteFile(cacheVersionFilePath, []byte(version), 0644) + + } else { + isAccessible, err := isAccessibleDirectory(cacheDirectory.path) + if err != nil { + return err + } + if !isAccessible { + return errors.Wrap(err, "Cache dir exists, but the current user can't write to it.") + } + + isEmpty, err := isEmptyDirectory(cacheDirectory.path) if err != nil { - return errors.Wrap(err, "Could not create cache version file.") + return err + } + if isEmpty { + err = ioutil.WriteFile(cacheVersionFilePath, []byte(version), 0644) + if err != nil { + return errors.Wrap(err, "Could not create cache version file.") + } } return nil + } return usererrors.New(errorNotACacheOrEmpty) } From 603d85137ebe6fcd60ed18d6fd1d633950a1945a Mon Sep 17 00:00:00 2001 From: Marcello DeSales Date: Tue, 28 Jan 2025 17:10:09 -0800 Subject: [PATCH 2/4] :hamster: :bug: fix the check for the empty dir --- internal/cachedirectory/cachedirectory.go | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/internal/cachedirectory/cachedirectory.go b/internal/cachedirectory/cachedirectory.go index 3644ac5..76840c5 100644 --- a/internal/cachedirectory/cachedirectory.go +++ b/internal/cachedirectory/cachedirectory.go @@ -2,7 +2,6 @@ package cachedirectory import ( usererrors "errors" - "io" "io/ioutil" "os" "path" @@ -108,19 +107,17 @@ func (cacheDirectory *CacheDirectory) CheckOrCreateVersionFile(pull bool, versio if !isAccessible { return errors.Wrap(err, "Cache dir exists, but the current user can't write to it.") } - - isEmpty, err := isEmptyDirectory(cacheDirectory.path) + } + isEmpty, err := isEmptyDirectory(cacheDirectory.path) + if err != nil { + return err + } + if isEmpty { + err = ioutil.WriteFile(cacheVersionFilePath, []byte(version), 0644) if err != nil { - return err - } - if isEmpty { - err = ioutil.WriteFile(cacheVersionFilePath, []byte(version), 0644) - if err != nil { - return errors.Wrap(err, "Could not create cache version file.") - } + return errors.Wrap(err, "Could not create cache version file.") } return nil - } return usererrors.New(errorNotACacheOrEmpty) } From b84492c2f8b363e1dc9408460df4ca08747827b0 Mon Sep 17 00:00:00 2001 From: Marcello DeSales Date: Tue, 28 Jan 2025 17:16:02 -0800 Subject: [PATCH 3/4] :hamster: :white_check_mark: add test case for empty cache dir The test covers the new case when an empty cache directory is provided and a new cache file is created. $ go test -v ./internal/cachedirectory -test.run ^\QTestUseProvidedEmptyCacheDirectory\E$ testing: warning: no tests to run PASS ok github.com/github/codeql-action-sync/internal/cachedirectory 0.932s [no tests to run] --- internal/cachedirectory/cachedirectory_test.go | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/internal/cachedirectory/cachedirectory_test.go b/internal/cachedirectory/cachedirectory_test.go index 8345cb5..b62b858 100644 --- a/internal/cachedirectory/cachedirectory_test.go +++ b/internal/cachedirectory/cachedirectory_test.go @@ -89,6 +89,18 @@ func TestCreateCacheDirectoryWithTrailingSlash(t *testing.T) { require.NoError(t, err) } +func TestUseProvidedEmptyCacheDirectory(t *testing.T) { + temporaryDirectory := test.CreateTemporaryDirectory(t) + cacheDirectoryPath := path.Join(temporaryDirectory, "cache") + err := os.MkdirAll(cacheDirectoryPath, 0755) + require.NoError(t, err) + cacheDirectory := NewCacheDirectory(cacheDirectoryPath) + err = cacheDirectory.CheckOrCreateVersionFile(true, aVersion) + require.NoError(t, err) + cacheVersionFilePath := cacheDirectory.versionFilePath() + require.FileExists(t, cacheVersionFilePath) +} + func TestLocking(t *testing.T) { temporaryDirectory := test.CreateTemporaryDirectory(t) cacheDirectory := NewCacheDirectory(path.Join(temporaryDirectory, "cache")) From 6f772780d2c27adacdf7320dd0913192233d8637 Mon Sep 17 00:00:00 2001 From: Chris Gavin Date: Tue, 4 Feb 2025 15:52:06 +0000 Subject: [PATCH 4/4] =?UTF-8?q?Fix=20horribly=20confusing=20lint=20failure?= =?UTF-8?q?.=20=F0=9F=98=85?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- internal/cachedirectory/cachedirectory.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/cachedirectory/cachedirectory.go b/internal/cachedirectory/cachedirectory.go index 76840c5..70f21e8 100644 --- a/internal/cachedirectory/cachedirectory.go +++ b/internal/cachedirectory/cachedirectory.go @@ -35,7 +35,7 @@ func isAccessibleDirectory(path string) (bool, error) { } return false, errors.Wrapf(err, "Could not access directory %s.", path) } - + return true, nil }