Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
20 changes: 12 additions & 8 deletions pkg/app/piped/imagewatcher/watcher.go
Original file line number Diff line number Diff line change
Expand Up @@ -114,11 +114,16 @@ func (w *watcher) run(ctx context.Context, provider imageprovider.Provider, inte
updates = append(updates, u...)
}
if len(updates) == 0 {
w.logger.Info("no image to be updated")
w.logger.Info("no image to be updated",
zap.String("image-provider", provider.Name()),
)
continue
}
if err := update(updates); err != nil {
w.logger.Error("failed to update image", zap.Error(err))
w.logger.Error("failed to update image",
zap.String("image-provider", provider.Name()),
zap.Error(err),
)
continue
}
}
Expand All @@ -136,14 +141,13 @@ func (w *watcher) determineUpdates(ctx context.Context, repoID string, repo git.
}

// Load Image Watcher Config for the given repo.
includes := make([]string, 0)
excludes := make([]string, 0)
var includes, excludes []string
for _, target := range w.config.ImageWatcher.Repos {
if target.RepoID != repoID {
continue
if target.RepoID == repoID {
includes = target.Includes
excludes = target.Excludes
break
}
includes = append(includes, target.Includes...)
excludes = append(excludes, target.Excludes...)
}
cfg, ok, err := config.LoadImageWatcher(repo.GetPath(), includes, excludes)
if err != nil {
Expand Down
2 changes: 2 additions & 0 deletions pkg/config/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ go_test(
"deployment_kubernetes_test.go",
"deployment_terraform_test.go",
"deployment_test.go",
"image_watcher_test.go",
"piped_test.go",
"replicas_test.go",
"sealed_secret_test.go",
Expand All @@ -47,6 +48,7 @@ go_test(
deps = [
"//pkg/model:go_default_library",
"@com_github_golang_protobuf//proto:go_default_library",
"@com_github_magiconair_properties//assert:go_default_library",
"@com_github_stretchr_testify//assert:go_default_library",
"@com_github_stretchr_testify//require:go_default_library",
],
Expand Down
84 changes: 79 additions & 5 deletions pkg/config/image_watcher.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,13 @@

package config

import (
"fmt"
"io/ioutil"
"os"
"path/filepath"
)

type ImageWatcherSpec struct {
Targets []ImageWatcherTarget `json:"targets"`
}
Expand All @@ -25,12 +32,79 @@ type ImageWatcherTarget struct {
Field string `json:"field"`
}

// LoadImageWatcher finds the config files for the image watcher in the .pipe directory first up.
// And returns parsed config, False is returned as the second returned value if not found.
// LoadImageWatcher finds the config files for the image watcher in the .pipe
// directory first up. And returns parsed config after merging the targets.
// Only one of includes or excludes can be used.
// False is returned as the second returned value if not found.
func LoadImageWatcher(repoRoot string, includes, excludes []string) (*ImageWatcherSpec, bool, error) {
// TODO: Load image watcher config
// referring to AnalysisTemplateSpec
return nil, false, nil
dir := filepath.Join(repoRoot, SharedConfigurationDirName)
files, err := ioutil.ReadDir(dir)
if os.IsNotExist(err) {
return nil, false, nil
}
if err != nil {
return nil, false, fmt.Errorf("failed to read %s: %w", dir, err)
Copy link
Member

Choose a reason for hiding this comment

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

I think no .pipe directory is a normal case. In that case, is piped sending this error to the log system?

Copy link
Member Author

Choose a reason for hiding this comment

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

Exactly, it should be handled as not found case.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should compare with os.IsNotExist.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right

}

spec := &ImageWatcherSpec{
Targets: make([]ImageWatcherTarget, 0),
}
filtered, err := filterImageWatcherFiles(files, includes, excludes)
if err != nil {
return nil, false, fmt.Errorf("failed to filter image watcher files at %s: %w", dir, err)
}
for _, f := range filtered {
if f.IsDir() {
continue
}
path := filepath.Join(dir, f.Name())
cfg, err := LoadFromYAML(path)
if err != nil {
return nil, false, fmt.Errorf("failed to load config file %s: %w", path, err)
}
if cfg.Kind == KindImageWatcher {
spec.Targets = append(spec.Targets, cfg.ImageWatcherSpec.Targets...)
}
}
if len(spec.Targets) == 0 {
return nil, false, nil
}

return spec, true, nil
}

// filterImageWatcherFiles filters the given files based on the given Includes and Excludes.
// Excludes are prioritized if both Excludes and Includes are given.
func filterImageWatcherFiles(files []os.FileInfo, includes, excludes []string) ([]os.FileInfo, error) {
if len(includes) == 0 && len(excludes) == 0 {
return files, nil
}

filtered := make([]os.FileInfo, 0, len(files))
useWhitelist := len(includes) != 0 && len(excludes) == 0
if useWhitelist {
whiteList := make(map[string]struct{}, len(includes))
for _, i := range includes {
whiteList[i] = struct{}{}
}
for _, f := range files {
if _, ok := whiteList[f.Name()]; ok {
filtered = append(filtered, f)
}
}
Copy link
Member

Choose a reason for hiding this comment

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

return to avoid else

Copy link
Member Author

@nakabonne nakabonne Dec 15, 2020

Choose a reason for hiding this comment

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

I expected it will be pointed out by you lol
It's tough to go out into it in English, but I'd say these processes are quite similar, so using if-else makes it readble. But it depends on the reader. What do you think?

Copy link
Member

@nghialv nghialv Dec 15, 2020

Choose a reason for hiding this comment

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

😄
I always follow return-early and avoid-deep-nesting.
Because when we see a return we don't need to care about all of the later parts.
Using if-else we don't have to care about the else's inside but still have to check the part after that else.

Copy link
Member Author

Choose a reason for hiding this comment

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

Gotcha 😄
No objection with that philosophy!

return filtered, nil
}

blackList := make(map[string]struct{}, len(excludes))
for _, e := range excludes {
blackList[e] = struct{}{}
}
for _, f := range files {
if _, ok := blackList[f.Name()]; !ok {
filtered = append(filtered, f)
}
}
return filtered, nil
}

func (s *ImageWatcherSpec) Validate() error {
Expand Down
126 changes: 126 additions & 0 deletions pkg/config/image_watcher_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,126 @@
// Copyright 2020 The PipeCD Authors.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package config

import (
"os"
"testing"
"time"

"github.com/stretchr/testify/assert"
)

type fakeFileInfo struct {
name string
}

func (f *fakeFileInfo) Name() string { return f.name }

// Below methods are required to meet the interface.
func (f *fakeFileInfo) Size() int64 { return 0 }
func (f *fakeFileInfo) Mode() os.FileMode { return 0 }
func (f *fakeFileInfo) ModTime() time.Time { return time.Now() }
func (f *fakeFileInfo) IsDir() bool { return false }
func (f *fakeFileInfo) Sys() interface{} { return nil }

func TestFilterImageWatcherFiles(t *testing.T) {
testcases := []struct {
name string
files []os.FileInfo
includes []string
excludes []string
want []os.FileInfo
wantErr bool
}{
{
name: "both includes and excludes aren't given",
files: []os.FileInfo{
&fakeFileInfo{
name: "file-1",
},
},
want: []os.FileInfo{
&fakeFileInfo{
name: "file-1",
},
},
wantErr: false,
},
{
name: "both includes and excludes are given",
files: []os.FileInfo{
&fakeFileInfo{
name: "file-1",
},
},
want: []os.FileInfo{},
includes: []string{"file-1"},
excludes: []string{"file-1"},
wantErr: false,
},
{
name: "includes given",
files: []os.FileInfo{
&fakeFileInfo{
name: "file-1",
},
&fakeFileInfo{
name: "file-2",
},
&fakeFileInfo{
name: "file-3",
},
},
includes: []string{"file-1", "file-3"},
want: []os.FileInfo{
&fakeFileInfo{
name: "file-1",
},
&fakeFileInfo{
name: "file-3",
},
},
wantErr: false,
},
{
name: "excludes given",
files: []os.FileInfo{
&fakeFileInfo{
name: "file-1",
},
&fakeFileInfo{
name: "file-2",
},
&fakeFileInfo{
name: "file-3",
},
},
excludes: []string{"file-1", "file-3"},
want: []os.FileInfo{
&fakeFileInfo{
name: "file-2",
},
},
wantErr: false,
},
}
for _, tc := range testcases {
t.Run(tc.name, func(t *testing.T) {
got, err := filterImageWatcherFiles(tc.files, tc.includes, tc.excludes)
assert.Equal(t, tc.wantErr, err != nil)
assert.Equal(t, tc.want, got)
})
}
}
16 changes: 16 additions & 0 deletions pkg/config/piped.go
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,9 @@ func (s *PipedSpec) Validate() error {
return err
}
}
if err := s.ImageWatcher.Validate(); err != nil {
return err
}
return nil
}

Expand Down Expand Up @@ -590,10 +593,23 @@ type PipedImageWatcher struct {
Repos []PipedImageWatcherRepoTarget `json:"repos"`
}

// Validate checks if the duplicated repository setting exists.
func (i *PipedImageWatcher) Validate() error {
repos := make(map[string]struct{})
for _, repo := range i.Repos {
if _, ok := repos[repo.RepoID]; ok {
return fmt.Errorf("duplicated repo id (%s) found in the imageWatcher directive", repo.RepoID)
}
repos[repo.RepoID] = struct{}{}
}
return nil
}

type PipedImageWatcherRepoTarget struct {
RepoID string `json:"repoId"`
// The paths to ImageWatcher files to be included.
Includes []string `json:"includes"`
// The paths to ImageWatcher files to be excluded.
// This is prioritized if both includes and this are given.
Excludes []string `json:"excludes"`
}
41 changes: 40 additions & 1 deletion pkg/config/piped_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -229,7 +229,7 @@ func TestPipedConfig(t *testing.T) {
Repos: []PipedImageWatcherRepoTarget{
{
RepoID: "foo",
Includes: []string{".pipe/imagewatcher-dev.yaml"},
Includes: []string{"imagewatcher-dev.yaml", "imagewatcher-stg.yaml"},
},
},
},
Expand All @@ -249,3 +249,42 @@ func TestPipedConfig(t *testing.T) {
})
}
}

func TestPipedImageWatcherValidate(t *testing.T) {
testcases := []struct {
name string
imageWatcher PipedImageWatcher
wantErr bool
}{
{
name: "duplicated repo exists",
wantErr: true,
imageWatcher: PipedImageWatcher{Repos: []PipedImageWatcherRepoTarget{
{
RepoID: "foo",
},
{
RepoID: "foo",
},
}},
},
{
name: "repos are unique",
wantErr: false,
imageWatcher: PipedImageWatcher{Repos: []PipedImageWatcherRepoTarget{
{
RepoID: "foo",
},
{
RepoID: "bar",
},
}},
},
}
for _, tc := range testcases {
t.Run(tc.name, func(t *testing.T) {
err := tc.imageWatcher.Validate()
assert.Equal(t, tc.wantErr, err != nil)
})
}
}
3 changes: 2 additions & 1 deletion pkg/config/testdata/piped/piped-config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -140,4 +140,5 @@ spec:
repos:
- repoId: foo
includes:
- .pipe/imagewatcher-dev.yaml
- imagewatcher-dev.yaml
- imagewatcher-stg.yaml