From 34cfabb56d5b1501240228de7c4aa5075a55803f Mon Sep 17 00:00:00 2001 From: Anbraten Date: Mon, 30 Aug 2021 22:54:21 +0200 Subject: [PATCH] Fix filter pipeline config files (#279) closes #271 - filter pipeline config folders for `.yml` and `.yaml` files - improve `fetchConfig` tests - update remote mock and correct wrong folder name `mock` => `mocks` to match package name - fix: return correct filename for fallback - improve config loading by checking if folder or not before sending api call --- remote/{mock/remote.go => mocks/Remote.go} | 24 +-- server/configFetcher.go | 59 ++++--- server/configFetcher_test.go | 188 +++++++++++++++++++-- 3 files changed, 229 insertions(+), 42 deletions(-) rename remote/{mock/remote.go => mocks/Remote.go} (94%) diff --git a/remote/mock/remote.go b/remote/mocks/Remote.go similarity index 94% rename from remote/mock/remote.go rename to remote/mocks/Remote.go index 0881c6a911..154a85f2de 100644 --- a/remote/mock/remote.go +++ b/remote/mocks/Remote.go @@ -1,11 +1,15 @@ -// Code generated by mockery v1.0.0. DO NOT EDIT. +// Code generated by mockery v0.0.0-dev. DO NOT EDIT. package mocks -import http "net/http" -import mock "github.com/stretchr/testify/mock" -import model "github.com/woodpecker-ci/woodpecker/model" -import remote "github.com/woodpecker-ci/woodpecker/remote" +import ( + http "net/http" + + mock "github.com/stretchr/testify/mock" + model "github.com/woodpecker-ci/woodpecker/model" + + remote "github.com/woodpecker-ci/woodpecker/remote" +) // Remote is an autogenerated mock type for the Remote type type Remote struct { @@ -254,13 +258,13 @@ func (_m *Remote) Repos(u *model.User) ([]*model.Repo, error) { return r0, r1 } -// Status provides a mock function with given fields: u, r, b, link -func (_m *Remote) Status(u *model.User, r *model.Repo, b *model.Build, link string) error { - ret := _m.Called(u, r, b, link) +// Status provides a mock function with given fields: u, r, b, link, proc +func (_m *Remote) Status(u *model.User, r *model.Repo, b *model.Build, link string, proc *model.Proc) error { + ret := _m.Called(u, r, b, link, proc) var r0 error - if rf, ok := ret.Get(0).(func(*model.User, *model.Repo, *model.Build, string) error); ok { - r0 = rf(u, r, b, link) + if rf, ok := ret.Get(0).(func(*model.User, *model.Repo, *model.Build, string, *model.Proc) error); ok { + r0 = rf(u, r, b, link, proc) } else { r0 = ret.Error(0) } diff --git a/server/configFetcher.go b/server/configFetcher.go index 5b79d1a536..f7591102dc 100644 --- a/server/configFetcher.go +++ b/server/configFetcher.go @@ -24,39 +24,58 @@ func NewConfigFetcher(remote remote.Remote, user *model.User, repo *model.Repo, } } -func (cf *configFetcher) Fetch() ([]*remote.FileMeta, error) { +func (cf *configFetcher) Fetch() (files []*remote.FileMeta, err error) { + var file []byte + for i := 0; i < 5; i++ { select { case <-time.After(time.Second * time.Duration(i)): + // either a file - file, fileerr := cf.remote_.File(cf.user, cf.repo, cf.build, cf.repo.Config) - if fileerr == nil { - return []*remote.FileMeta{{ - Name: cf.repo.Config, - Data: file, - }}, nil + if !strings.HasSuffix(cf.repo.Config, "/") { + file, err = cf.remote_.File(cf.user, cf.repo, cf.build, cf.repo.Config) + if err == nil { + return []*remote.FileMeta{{ + Name: cf.repo.Config, + Data: file, + }}, nil + } } // or a folder - dir, direrr := cf.remote_.Dir(cf.user, cf.repo, cf.build, strings.TrimSuffix(cf.repo.Config, "/")) - - if direrr == nil { - return dir, nil - } else if !cf.repo.Fallback { - return nil, direrr + if strings.HasSuffix(cf.repo.Config, "/") { + files, err = cf.remote_.Dir(cf.user, cf.repo, cf.build, strings.TrimSuffix(cf.repo.Config, "/")) + if err == nil { + return filterPipelineFiles(files), nil + } } // or fallback - file, fileerr = cf.remote_.File(cf.user, cf.repo, cf.build, ".drone.yml") - if fileerr != nil { - return nil, fileerr + if cf.repo.Fallback { + file, err = cf.remote_.File(cf.user, cf.repo, cf.build, ".drone.yml") + if err == nil { + return []*remote.FileMeta{{ + Name: ".drone.yml", + Data: file, + }}, nil + } } - return []*remote.FileMeta{{ - Name: cf.repo.Config, - Data: file, - }}, nil + return nil, err } } + return []*remote.FileMeta{}, nil } + +func filterPipelineFiles(files []*remote.FileMeta) []*remote.FileMeta { + var res []*remote.FileMeta + + for _, file := range files { + if strings.HasSuffix(file.Name, ".yml") || strings.HasSuffix(file.Name, ".yaml") { + res = append(res, file) + } + } + + return res +} diff --git a/server/configFetcher_test.go b/server/configFetcher_test.go index cf17248340..84b5de7949 100644 --- a/server/configFetcher_test.go +++ b/server/configFetcher_test.go @@ -1,25 +1,189 @@ package server_test import ( + "errors" "testing" + "github.com/stretchr/testify/mock" "github.com/woodpecker-ci/woodpecker/model" - "github.com/woodpecker-ci/woodpecker/remote/github" + "github.com/woodpecker-ci/woodpecker/remote" + "github.com/woodpecker-ci/woodpecker/remote/mocks" "github.com/woodpecker-ci/woodpecker/server" ) -func TestFetchGithub(t *testing.T) { +func TestFetch(t *testing.T) { t.Parallel() - github, err := github.New(github.Opts{URL: "https://github.com"}) - if err != nil { - t.Fatal(err) + testTable := []struct { + name string + repoConfig string + repoFallback bool + fileMocks []struct { + file []byte + err error + } + dirMock struct { + files []*remote.FileMeta + err error + } + expectedFileNames []string + expectedError bool + }{ + { + name: "Single .woodpecker.yml file", + repoConfig: ".woodpecker.yml", + repoFallback: false, + fileMocks: []struct { + file []byte + err error + }{ + { + file: []byte{}, + err: nil, + }, + }, + expectedFileNames: []string{ + ".woodpecker.yml", + }, + expectedError: false, + }, + { + name: "Folder .woodpecker/", + repoConfig: ".woodpecker/", + repoFallback: false, + dirMock: struct { + files []*remote.FileMeta + err error + }{ + files: []*remote.FileMeta{ + { + Name: ".woodpecker/text.txt", + Data: []byte{}, + }, + { + Name: ".woodpecker/release.yml", + Data: []byte{}, + }, + { + Name: ".woodpecker/image.png", + Data: []byte{}, + }, + }, + err: nil, + }, + expectedFileNames: []string{ + ".woodpecker/release.yml", + }, + expectedError: false, + }, + { + name: "Requesting woodpecker-file but using fallback", + repoConfig: ".woodpecker.yml", + repoFallback: true, + fileMocks: []struct { + file []byte + err error + }{ + // first call requesting regular woodpecker.yml + { + file: nil, + err: errors.New("File not found"), + }, + // fallback file call + { + file: []byte{}, + err: nil, + }, + }, + expectedFileNames: []string{ + ".drone.yml", + }, + expectedError: false, + }, + { + name: "Requesting folder but using fallback", + repoConfig: ".woodpecker/", + repoFallback: true, + fileMocks: []struct { + file []byte + err error + }{ + { + file: []byte{}, + err: nil, + }, + }, + dirMock: struct { + files []*remote.FileMeta + err error + }{ + files: []*remote.FileMeta{}, + err: errors.New("Dir not found"), + }, + expectedFileNames: []string{ + ".drone.yml", + }, + expectedError: false, + }, + { + name: "Not found and disabled fallback", + repoConfig: ".woodpecker.yml", + repoFallback: false, + fileMocks: []struct { + file []byte + err error + }{ + // first call requesting regular woodpecker.yml + { + file: nil, + err: errors.New("File not found"), + }, + // fallback file call + { + file: []byte{}, + err: errors.New("File not found"), + }, + }, + expectedFileNames: []string{}, + expectedError: true, + }, + } + + for _, tt := range testTable { + t.Run(tt.name, func(t *testing.T) { + repo := &model.Repo{Owner: "laszlocph", Name: "drone-multipipeline", Config: tt.repoConfig, Fallback: tt.repoFallback} + + r := new(mocks.Remote) + for _, fileMock := range tt.fileMocks { + r.On("File", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(fileMock.file, fileMock.err).Once() + } + r.On("Dir", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(tt.dirMock.files, tt.dirMock.err) + + configFetcher := server.NewConfigFetcher( + r, + &model.User{Token: "xxx"}, + repo, + &model.Build{Commit: "89ab7b2d6bfb347144ac7c557e638ab402848fee"}, + ) + files, err := configFetcher.Fetch() + if tt.expectedError && err == nil { + t.Fatal("expected an error") + } else if !tt.expectedError && err != nil { + t.Fatal("error fetching config:", err) + } + + matchingFiles := 0 + for _, expectedFileName := range tt.expectedFileNames { + for _, file := range files { + if file.Name == expectedFileName { + matchingFiles += 1 + } + } + } + + if matchingFiles != len(tt.expectedFileNames) { + t.Fatal("expected some other pipeline files", tt.expectedFileNames, files) + } + }) } - configFetcher := server.NewConfigFetcher( - github, - &model.User{Token: "xxx"}, - &model.Repo{Owner: "laszlocph", Name: "drone-multipipeline", Config: ".drone"}, - &model.Build{Commit: "89ab7b2d6bfb347144ac7c557e638ab402848fee"}, - ) - configFetcher.Fetch() }