Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: clean paths after reading config #341

Merged
merged 3 commits into from
Jan 30, 2024
Merged

fix: clean paths after reading config #341

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
80a2003
fix: clean paths after reading config
Zebradil Dec 24, 2023
d024e8e
refactor: revert changes to grouping of imports
Zebradil Jan 10, 2024
52843a3
test: add unit test to cover path cleaning
Zebradil Jan 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions pkg/vendir/config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ func NewConfigFromFiles(paths []string) (Config, []Secret, []ConfigMap, error) {

case res.APIVersion == knownAPIVersion && res.Kind == knownKind:
config, err := NewConfigFromBytes(docBytes)
config.cleanPaths()
if err != nil {
return fmt.Errorf("Unmarshaling config: %s", err)
}
Expand Down Expand Up @@ -278,3 +279,12 @@ func (c Config) checkOverlappingPaths() error {

return nil
}

func (c *Config) cleanPaths() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we are change the configuration directly will this have any impact on the lazy feature?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand the question.

Paths are clean up right after reading the config, so after that any operation uses so to say "canonical" path, including the lock file and lazy functionality.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But if the lock file already exists, the path might be different correct?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But if the lock file already exists, the path might be different correct?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the current vendir behavior is to copy paths from the config to the lock file literally. With the cleanup logic, paths can differ between the config and the lock file. It is basically the same as changing vendor/ to vendor in the config and run sync. The lock file will be updated with the new path.

So, yes, if this is merged, the next sync with the new version of vendir will invalidate the "lazy cache" in some cases. But I don't think that can be an issue.

for i, dir := range c.Directories {
c.Directories[i].Path = filepath.Clean(dir.Path)
for j, con := range dir.Contents {
c.Directories[i].Contents[j].Path = filepath.Clean(con.Path)
}
}
}
68 changes: 68 additions & 0 deletions pkg/vendir/config/config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,74 @@ kind: Config`)
})
}

func TestCleanPaths(t *testing.T) {
cases := []struct {
name string
input string
checkFn func(config.Config)
}{
{
name: "single directory and single contents",
input: `
apiVersion: vendir.k14s.io/v1alpha1
kind: Config
directories:
- path: vendor/foo/..//
contents:
- path: bar//baz///../
inline:
paths:
file.txt: File contents
`,
checkFn: func(cfg config.Config) {
require.Equal(t, "vendor", cfg.Directories[0].Path)
require.Equal(t, "bar", cfg.Directories[0].Contents[0].Path)
},
},
{
name: "multiple directories and multiple contents",
input: `
apiVersion: vendir.k14s.io/v1alpha1
kind: Config
directories:
- path: vendor/foo/
contents:
- path: bar///baz/.
inline:
paths:
file.txt: File contents
- path: lorem/ipsum
inline:
paths:
file.txt: File contents
- path: vendor//../vendor/bar
contents:
- path: baz
inline:
paths:
file.txt: File contents
`,
checkFn: func(cfg config.Config) {
require.Equal(t, "vendor/foo", cfg.Directories[0].Path)
require.Equal(t, "bar/baz", cfg.Directories[0].Contents[0].Path)
require.Equal(t, "lorem/ipsum", cfg.Directories[0].Contents[1].Path)
require.Equal(t, "vendor/bar", cfg.Directories[1].Path)
require.Equal(t, "baz", cfg.Directories[1].Contents[0].Path)
},
},
}

for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
tmpFile := filepath.Join(t.TempDir(), "config.yml")
require.NoError(t, os.WriteFile(tmpFile, []byte(tc.input), 0600))
cfg, _, _, err := config.NewConfigFromFiles([]string{tmpFile})
require.NoError(t, err)
tc.checkFn(cfg)
})
}
}

func TestSecretsForNewConfigFromFiles(t *testing.T) {
t.Run("Config with single secret", func(t *testing.T) {
tempConfigPath := filepath.Join(t.TempDir(), "config.yml")
Expand Down
Loading