From 8d6da3b659edd9be934733ec9a26c3a7f56319bc Mon Sep 17 00:00:00 2001 From: Fritz Duchardt Date: Tue, 14 Nov 2023 08:48:07 +0100 Subject: [PATCH] fix: fixing error when running vendir sync with directory option while corresponding entry is missing from vendir.lock.yaml Signed-off-by: Fritz Duchardt --- examples/git-and-manual/local-dir-2/file.txt | 1 + .../git-and-manual/local-dir-dev/file.txt | 2 +- examples/git-and-manual/vendir.lock.yml | 2 + examples/git-and-manual/vendir.yml | 3 + .../vendor/local-dir-2/file.txt | 1 + pkg/vendir/cmd/sync.go | 27 ++++- pkg/vendir/config/config.go | 12 +- pkg/vendir/config/lock_config.go | 49 ++++---- pkg/vendir/config/lock_config_test.go | 106 ++++++++++++++++++ pkg/vendir/directory/directory.go | 16 ++- pkg/vendir/directory/staging_dir.go | 41 +++++-- test/e2e/directory_flag_test.go | 42 +++---- 12 files changed, 234 insertions(+), 68 deletions(-) create mode 100644 examples/git-and-manual/local-dir-2/file.txt create mode 100644 examples/git-and-manual/vendor/local-dir-2/file.txt diff --git a/examples/git-and-manual/local-dir-2/file.txt b/examples/git-and-manual/local-dir-2/file.txt new file mode 100644 index 00000000..03fd717e --- /dev/null +++ b/examples/git-and-manual/local-dir-2/file.txt @@ -0,0 +1 @@ +file-dir-2 diff --git a/examples/git-and-manual/local-dir-dev/file.txt b/examples/git-and-manual/local-dir-dev/file.txt index bc524441..75a3d7d0 100644 --- a/examples/git-and-manual/local-dir-dev/file.txt +++ b/examples/git-and-manual/local-dir-dev/file.txt @@ -1 +1 @@ -local-dir2/file +file-dir-dev diff --git a/examples/git-and-manual/vendir.lock.yml b/examples/git-and-manual/vendir.lock.yml index 0fe0649b..62a4d3cb 100644 --- a/examples/git-and-manual/vendir.lock.yml +++ b/examples/git-and-manual/vendir.lock.yml @@ -11,5 +11,7 @@ directories: path: github.com/GoogleCloudPlatform/metacontroller - directory: {} path: local-dir + - directory: {} + path: local-dir-2 path: vendor kind: LockConfig diff --git a/examples/git-and-manual/vendir.yml b/examples/git-and-manual/vendir.yml index c4b91afc..18c5e9f1 100644 --- a/examples/git-and-manual/vendir.yml +++ b/examples/git-and-manual/vendir.yml @@ -17,3 +17,6 @@ directories: - path: local-dir directory: path: local-dir + - path: local-dir-2 + directory: + path: local-dir-2 diff --git a/examples/git-and-manual/vendor/local-dir-2/file.txt b/examples/git-and-manual/vendor/local-dir-2/file.txt new file mode 100644 index 00000000..03fd717e --- /dev/null +++ b/examples/git-and-manual/vendor/local-dir-2/file.txt @@ -0,0 +1 @@ +file-dir-2 diff --git a/pkg/vendir/cmd/sync.go b/pkg/vendir/cmd/sync.go index 4e5d728b..b2d8d757 100644 --- a/pkg/vendir/cmd/sync.go +++ b/pkg/vendir/cmd/sync.go @@ -36,6 +36,13 @@ type SyncOptions struct { AllowAllSymlinkDestinations bool } +func (o *SyncOptions) LockFileExists() bool { + if _, err := os.Stat(o.LockFile); err != nil { + return false + } + return true +} + func NewSyncOptions(ui ui.UI) *SyncOptions { return &SyncOptions{ui: ui} } @@ -133,13 +140,15 @@ func (o *SyncOptions) Run() error { HelmBinary: os.Getenv("VENDIR_HELM_BINARY"), Cache: cache, Lazy: o.Lazy, + Partial: len(dirs) > 0, } newLockConfig := ctlconf.NewLockConfig() for _, dirConf := range conf.Directories { // error safe to ignore, since lock file might not exist dirExistingLockConf, _ := existingLockConfig.FindDirectory(dirConf.Path) - dirLockConf, err := ctldir.NewDirectory(dirConf, dirExistingLockConf, o.ui).Sync(syncOpts) + directory := ctldir.NewDirectory(dirConf, dirExistingLockConf, o.ui) + dirLockConf, err := directory.Sync(syncOpts) if err != nil { return fmt.Errorf("Syncing directory '%s': %s", dirConf.Path, err) } @@ -155,12 +164,18 @@ func (o *SyncOptions) Run() error { // Update only selected directories in lock file if len(dirs) > 0 { - err = existingLockConfig.Merge(newLockConfig) - if err != nil { - return err - } + if o.LockFileExists() { + existingLockConfig, err := ctlconf.NewLockConfigFromFile(o.LockFile) + if err != nil { + return err + } + err = existingLockConfig.Merge(newLockConfig) + if err != nil { + return err + } - newLockConfig = existingLockConfig + newLockConfig = existingLockConfig + } } newLockConfigBs, err := newLockConfig.AsBytes() diff --git a/pkg/vendir/config/config.go b/pkg/vendir/config/config.go index 41dfc5a2..a591d338 100644 --- a/pkg/vendir/config/config.go +++ b/pkg/vendir/config/config.go @@ -209,22 +209,22 @@ func (c Config) Subset(paths []string) (Config, error) { for _, dir := range c.Directories { for _, con := range dir.Contents { - path := filepath.Join(dir.Path, con.Path) + entirePath := filepath.Join(dir.Path, con.Path) - seen, found := pathsToSeen[path] + seen, found := pathsToSeen[entirePath] if !found { continue } if seen { - return Config{}, fmt.Errorf("Expected to match path '%s' once, but matched multiple", path) + return Config{}, fmt.Errorf("Expected to match path '%s' once, but matched multiple", entirePath) } - pathsToSeen[path] = true + pathsToSeen[entirePath] = true newCon := con // copy (but not deep unfortunately) - newCon.Path = EntireDirPath + newCon.Path = con.Path result.Directories = append(result.Directories, Directory{ - Path: path, + Path: dir.Path, Contents: []DirectoryContents{newCon}, }) } diff --git a/pkg/vendir/config/lock_config.go b/pkg/vendir/config/lock_config.go index c449ffff..6c4988ec 100644 --- a/pkg/vendir/config/lock_config.go +++ b/pkg/vendir/config/lock_config.go @@ -30,7 +30,10 @@ func NewLockConfig() LockConfig { func LockFileExists(path string) bool { if _, err := os.Stat(path); err != nil { - return false + if errors.Is(err, fs.ErrNotExist) { + return false + } + panic(fmt.Errorf("can not stat file '%v': %v", path, err)) } return true } @@ -131,42 +134,48 @@ func (c LockConfig) FindDirectory(dirPath string) (LockDirectory, error) { "Expected to find directory '%s' within lock config, but did not", dirPath) } -func (c LockConfig) Merge(other LockConfig) error { +func (c *LockConfig) Merge(other LockConfig) error { for _, dir := range other.Directories { + replaced := false for _, con := range dir.Contents { - err := c.MergeContents(filepath.Join(dir.Path, con.Path), con) - if err != nil { - return err + replaced = c.ReplaceContents(filepath.Join(dir.Path, con.Path), con) + if replaced { + continue + } + replaced = c.AppendContents(dir.Path, con) + if replaced { + continue } } + if !replaced { + c.Directories = append(c.Directories, dir) + } } return nil } -func (c LockConfig) MergeContents(path string, replaceCon LockDirectoryContents) error { - var matched bool +func (c *LockConfig) ReplaceContents(path string, replaceCon LockDirectoryContents) bool { for i, dir := range c.Directories { for j, con := range dir.Contents { if filepath.Join(dir.Path, con.Path) != path { continue } - - if matched { - return fmt.Errorf("Expected to match exactly one directory, but matched multiple") - } - matched = true - - newCon := replaceCon - newCon.Path = con.Path - - dir.Contents[j] = newCon + dir.Contents[j] = replaceCon c.Directories[i] = dir + return true } } - if !matched { - return fmt.Errorf("Expected to match exactly one directory, but did not match any") + return false +} + +func (c *LockConfig) AppendContents(path string, appendCon LockDirectoryContents) bool { + for i, dir := range c.Directories { + if dir.Path == path { + c.Directories[i].Contents = append(dir.Contents, appendCon) + return true + } } - return nil + return false } diff --git a/pkg/vendir/config/lock_config_test.go b/pkg/vendir/config/lock_config_test.go index 36fa32bf..10c73c9d 100644 --- a/pkg/vendir/config/lock_config_test.go +++ b/pkg/vendir/config/lock_config_test.go @@ -148,3 +148,109 @@ func TestWriteToFile(t *testing.T) { require.NoError(t, os.RemoveAll(tempDir)) } + +func TestLockConfig_ReplaceContents(t *testing.T) { + lockConfig := config.LockConfig{ + APIVersion: "vendir.k14s.io/v1alpha1", + Kind: "LockConfig", + Directories: []config.LockDirectory{ + { + Path: "dirpath", + Contents: []config.LockDirectoryContents{ + { + Path: "dirpath", + }, + }, + }, + }, + } + lockContents := config.LockDirectoryContents{ + Path: "dirpath", + HelmChart: &config.LockDirectoryContentsHelmChart{ + Version: "test", + }, + } + t.Run("replace contents", func(t *testing.T) { + appended := lockConfig.ReplaceContents("dirpath/dirpath", lockContents) + require.True(t, appended) + require.Equal(t, 1, len(lockConfig.Directories)) + require.Equal(t, 1, len(lockConfig.Directories[0].Contents)) + require.Equal(t, config.LockDirectoryContents{Path: "dirpath", HelmChart: &config.LockDirectoryContentsHelmChart{Version: "test"}}, lockConfig.Directories[0].Contents[0]) + }) + + t.Run("fail to append content to config due to wrong path", func(t *testing.T) { + appended := lockConfig.AppendContents("wrong-path", lockContents) + require.False(t, appended) + }) +} + +func TestLockConfig_AppendContents(t *testing.T) { + lockConfig := config.LockConfig{ + APIVersion: "vendir.k14s.io/v1alpha1", + Kind: "LockConfig", + Directories: []config.LockDirectory{ + { + Path: "dirpath", + Contents: []config.LockDirectoryContents{ + { + Path: "dirpath", + }, + }, + }, + }, + } + lockContents := config.LockDirectoryContents{ + Path: "gitpath", + } + t.Run("append contents to config", func(t *testing.T) { + appended := lockConfig.AppendContents("dirpath", lockContents) + require.True(t, appended) + require.Equal(t, 1, len(lockConfig.Directories)) + require.Equal(t, 2, len(lockConfig.Directories[0].Contents)) + require.Equal(t, config.LockDirectoryContents{Path: "gitpath"}, lockConfig.Directories[0].Contents[1]) + }) + + t.Run("fail to append content to config due to wrong path", func(t *testing.T) { + appended := lockConfig.AppendContents("wrong-path", lockContents) + require.False(t, appended) + }) +} + +func TestLockConfig_Merge(t *testing.T) { + lockConfig := config.LockConfig{ + APIVersion: "vendir.k14s.io/v1alpha1", + Kind: "LockConfig", + Directories: []config.LockDirectory{ + { + Path: "gitpath-1", + Contents: []config.LockDirectoryContents{ + { + Path: "gitpath-1", + }, + }, + }, + }, + } + lockConfig2 := config.LockConfig{ + APIVersion: "vendir.k14s.io/v1alpha1", + Kind: "LockConfig", + Directories: []config.LockDirectory{ + { + Path: "gitpath-2", + Contents: []config.LockDirectoryContents{ + { + Path: "gitpath-2", + }, + }, + }, + }, + } + t.Run("append directory to config", func(t *testing.T) { + lockConfig.Merge(lockConfig2) + require.Equal(t, 2, len(lockConfig.Directories)) + require.Equal(t, 1, len(lockConfig.Directories[0].Contents)) + require.Equal(t, 1, len(lockConfig.Directories[1].Contents)) + require.Equal(t, config.LockDirectoryContents{Path: "gitpath-1"}, lockConfig.Directories[0].Contents[0]) + require.Equal(t, config.LockDirectoryContents{Path: "gitpath-2"}, lockConfig.Directories[1].Contents[0]) + }) +} diff --git a/pkg/vendir/directory/directory.go b/pkg/vendir/directory/directory.go index 4477337f..6b354c4c 100644 --- a/pkg/vendir/directory/directory.go +++ b/pkg/vendir/directory/directory.go @@ -42,6 +42,7 @@ type SyncOpts struct { HelmBinary string Cache ctlcache.Cache Lazy bool + Partial bool } func createConfigDigest(contents ctlconf.DirectoryContents) (string, error) { @@ -251,11 +252,20 @@ func (d *Directory) Sync(syncOpts SyncOpts) (ctlconf.LockDirectory, error) { } lockConfig.Contents = append(lockConfig.Contents, lockDirContents) + + if syncOpts.Partial { + err = stagingDir.PartialRepace(contents.Path, filepath.Join(d.opts.Path, contents.Path)) + if err != nil { + return lockConfig, err + } + } } - err = stagingDir.Replace(d.opts.Path) - if err != nil { - return lockConfig, err + if !syncOpts.Partial { + err = stagingDir.Replace(d.opts.Path) + if err != nil { + return lockConfig, err + } } // after everything else is done, ensure the outer dir's access perms are set diff --git a/pkg/vendir/directory/staging_dir.go b/pkg/vendir/directory/staging_dir.go index f9bb1d8d..1e82c590 100644 --- a/pkg/vendir/directory/staging_dir.go +++ b/pkg/vendir/directory/staging_dir.go @@ -130,25 +130,50 @@ func isPathIgnored(path string, ignorePaths []string) (bool, error) { return false, nil } +// Replaces entire final location directory with staging directory func (d StagingDir) Replace(path string) error { - err := os.RemoveAll(path) + + err := d.prepareOutputDirectory(path) if err != nil { - return fmt.Errorf("Deleting dir %s: %s", path, err) + return err } - // Clean to avoid getting 'out/in/' from 'out/in/' instead of just 'out' - parentPath := filepath.Dir(filepath.Clean(path)) + err = os.Rename(d.stagingDir, path) + if err != nil { + return fmt.Errorf("Moving staging directory '%s' to final location '%s': %s", d.stagingDir, path, err) + } - err = os.MkdirAll(parentPath, 0700) + return nil +} + +// Replaces single directory of final location dir with single directory of staging dir +func (d StagingDir) PartialRepace(contentPath string, directoryPath string) error { + err := d.prepareOutputDirectory(directoryPath) if err != nil { - return fmt.Errorf("Creating final location parent dir %s: %s", parentPath, err) + return err } - err = os.Rename(d.stagingDir, path) + err = os.Rename(filepath.Join(d.stagingDir, contentPath), directoryPath) if err != nil { - return fmt.Errorf("Moving staging directory '%s' to final location '%s': %s", d.stagingDir, path, err) + return fmt.Errorf("Moving staging directory '%s' to final location '%s': %s", d.stagingDir, directoryPath, err) + } + + return nil +} + +func (d StagingDir) prepareOutputDirectory(directoryPath string) error { + err := os.RemoveAll(directoryPath) + if err != nil { + return fmt.Errorf("Deleting dir %s: %s", directoryPath, err) } + // Clean to avoid getting 'out/in/' from 'out/in/' instead of just 'out' + parentPath := filepath.Dir(filepath.Clean(directoryPath)) + + err = os.MkdirAll(parentPath, 0700) + if err != nil { + return fmt.Errorf("Creating final location parent dir %s: %s", parentPath, err) + } return nil } diff --git a/test/e2e/directory_flag_test.go b/test/e2e/directory_flag_test.go index e348a315..0906f2b6 100644 --- a/test/e2e/directory_flag_test.go +++ b/test/e2e/directory_flag_test.go @@ -4,6 +4,7 @@ package e2e import ( + "github.com/stretchr/testify/require" "os" "testing" ) @@ -18,37 +19,30 @@ func TestUseDirectory(t *testing.T) { reset := func() { // Make sure state is reset _, err := vendir.RunWithOpts([]string{"sync"}, RunOpts{Dir: path}) - if err != nil { - t.Fatalf("Expected no err") - } + require.NoError(t, err) } - - reset() - defer reset() - - checkFileContent := func(expectedContent string) { - file, err := os.ReadFile(path + "/vendor/local-dir/file.txt") - if err != nil { - t.Fatalf("Expected no err") - } - if string(file) != expectedContent { - t.Fatalf("Expected file contents to be known: %s", string(file)) - } + checkFileContent := func(filePath string, expectedContent string) { + file, err := os.ReadFile(path + filePath) + require.NoError(t, err) + require.EqualValues(t, string(file), expectedContent) } - checkFileContent("file\n") - - _, err := vendir.RunWithOpts([]string{"sync"}, RunOpts{Dir: path}) + // Sync with directory flag + _, err := vendir.RunWithOpts([]string{"sync", "--directory", "vendor/local-dir", "--directory", "vendor/local-dir-2"}, RunOpts{Dir: path}) if err != nil { t.Fatalf("Expected no err") } + checkFileContent("/vendor/local-dir/file.txt", "file\n") + checkFileContent("/vendor/local-dir-2/file.txt", "file-dir-2\n") - checkFileContent("file\n") - + // Sync with directory flag and local-dir-dev _, err = vendir.RunWithOpts([]string{"sync", "--directory", "vendor/local-dir=local-dir-dev"}, RunOpts{Dir: path}) - if err != nil { - t.Fatalf("Expected no err") - } + require.NoError(t, err) + checkFileContent("/vendor/local-dir/file.txt", "file-dir-dev\n") - checkFileContent("local-dir2/file\n") + // Lock file check + require.FileExists(t, path+"/vendir.lock.yml") + + // Final regular sync to make sure nothing in vendir was deleted (manual syncs would fail). + reset() }