From 5a05a7fe6bf40ed827e3da89653dd837e389df77 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Sun, 26 Sep 2021 01:23:17 +0200 Subject: [PATCH] Make configFetcher fallback work for gitea (#357) #299 who closed #133 did not take into account, that that gitea (and eventually) other forges do return 200 and empty string if file was not found - this make configFetcher more resilient --- server/shared/configFetcher.go | 21 ++-- server/shared/configFetcher_test.go | 170 +++++++++++++++++----------- 2 files changed, 121 insertions(+), 70 deletions(-) diff --git a/server/shared/configFetcher.go b/server/shared/configFetcher.go index 4343c8fcb..b2a6a5f88 100644 --- a/server/shared/configFetcher.go +++ b/server/shared/configFetcher.go @@ -1,6 +1,7 @@ package shared import ( + "fmt" "strings" "time" @@ -31,13 +32,13 @@ func (cf *configFetcher) Fetch() (files []*remote.FileMeta, err error) { for i := 0; i < 5; i++ { select { case <-time.After(time.Second * time.Duration(i)): - if len(cf.repo.Config) > 0 { + if len(config) > 0 { // either a file if !strings.HasSuffix(config, "/") { file, err = cf.remote_.File(cf.user, cf.repo, cf.build, config) - if err == nil { + if err == nil && len(file) != 0 { return []*remote.FileMeta{{ - Name: cf.repo.Config, + Name: config, Data: file, }}, nil } @@ -54,12 +55,13 @@ func (cf *configFetcher) Fetch() (files []*remote.FileMeta, err error) { // test .woodpecker/ folder // if folder is not supported we will get a "Not implemented" error and continue files, err = cf.remote_.Dir(cf.user, cf.repo, cf.build, ".woodpecker") - if err == nil { - return filterPipelineFiles(files), nil + files = filterPipelineFiles(files) + if err == nil && len(files) != 0 { + return files, nil } file, err = cf.remote_.File(cf.user, cf.repo, cf.build, ".woodpecker.yml") - if err == nil { + if err == nil && len(file) != 0 { return []*remote.FileMeta{{ Name: ".woodpecker.yml", Data: file, @@ -67,14 +69,19 @@ func (cf *configFetcher) Fetch() (files []*remote.FileMeta, err error) { } file, err = cf.remote_.File(cf.user, cf.repo, cf.build, ".drone.yml") - if err == nil { + if err == nil && len(file) != 0 { return []*remote.FileMeta{{ Name: ".drone.yml", Data: file, }}, nil } + + if err == nil && len(files) == 0 { + return nil, fmt.Errorf("ConfigFetcher: Fallback did not found config") + } } + // TODO: retry loop is inactive and could maybe be fixed/deleted return nil, err } } diff --git a/server/shared/configFetcher_test.go b/server/shared/configFetcher_test.go index eb605e583..effbafc7c 100644 --- a/server/shared/configFetcher_test.go +++ b/server/shared/configFetcher_test.go @@ -5,31 +5,45 @@ import ( "path/filepath" "testing" - "github.com/stretchr/testify/mock" "github.com/woodpecker-ci/woodpecker/model" "github.com/woodpecker-ci/woodpecker/server/remote" "github.com/woodpecker-ci/woodpecker/server/remote/mocks" "github.com/woodpecker-ci/woodpecker/server/shared" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" ) func TestFetch(t *testing.T) { t.Parallel() + type file struct { + name string + data []byte + } + + dummyData := []byte("TEST") + testTable := []struct { name string repoConfig string - files []string + files []file expectedFileNames []string expectedError bool }{ { name: "Default config - .woodpecker/", repoConfig: "", - files: []string{ - ".woodpecker/text.txt", - ".woodpecker/release.yml", - ".woodpecker/image.png", - }, + files: []file{{ + name: ".woodpecker/text.txt", + data: dummyData, + }, { + name: ".woodpecker/release.yml", + data: dummyData, + }, { + name: ".woodpecker/image.png", + data: dummyData, + }}, expectedFileNames: []string{ ".woodpecker/release.yml", }, @@ -38,9 +52,10 @@ func TestFetch(t *testing.T) { { name: "Default config - .woodpecker.yml", repoConfig: "", - files: []string{ - ".woodpecker.yml", - }, + files: []file{{ + name: ".woodpecker.yml", + data: dummyData, + }}, expectedFileNames: []string{ ".woodpecker.yml", }, @@ -49,9 +64,10 @@ func TestFetch(t *testing.T) { { name: "Default config - .drone.yml", repoConfig: "", - files: []string{ - ".drone.yml", - }, + files: []file{{ + name: ".drone.yml", + data: dummyData, + }}, expectedFileNames: []string{ ".drone.yml", }, @@ -60,17 +76,20 @@ func TestFetch(t *testing.T) { { name: "Default config - Empty repo", repoConfig: "", - files: []string{}, + files: []file{}, expectedFileNames: []string{}, expectedError: true, }, { name: "Default config - Additional sub-folders", repoConfig: "", - files: []string{ - ".woodpecker/test.yml", - ".woodpecker/sub-folder/config.yml", - }, + files: []file{{ + name: ".woodpecker/test.yml", + data: dummyData, + }, { + name: ".woodpecker/sub-folder/config.yml", + data: dummyData, + }}, expectedFileNames: []string{ ".woodpecker/test.yml", }, @@ -79,25 +98,55 @@ func TestFetch(t *testing.T) { { name: "Default config - Additional none .yml files", repoConfig: "", - files: []string{ - ".woodpecker/notes.txt", - ".woodpecker/image.png", - ".woodpecker/test.yml", - }, + files: []file{{ + name: ".woodpecker/notes.txt", + data: dummyData, + }, { + name: ".woodpecker/image.png", + data: dummyData, + }, { + name: ".woodpecker/test.yml", + data: dummyData, + }}, expectedFileNames: []string{ ".woodpecker/test.yml", }, expectedError: false, }, + { + name: "Default config - Empty Folder", + repoConfig: " ", + files: []file{{ + name: ".woodpecker/.keep", + data: dummyData, + }, { + name: ".woodpecker.yml", + data: nil, + }, { + name: ".drone.yml", + data: dummyData, + }}, + expectedFileNames: []string{ + ".drone.yml", + }, + expectedError: false, + }, { name: "Special config - folder (ignoring default files)", repoConfig: ".my-ci-folder/", - files: []string{ - ".woodpecker/test.yml", - ".woodpecker.yml", - ".drone.yml", - ".my-ci-folder/test.yml", - }, + files: []file{{ + name: ".woodpecker/test.yml", + data: dummyData, + }, { + name: ".woodpecker.yml", + data: dummyData, + }, { + name: ".drone.yml", + data: dummyData, + }, { + name: ".my-ci-folder/test.yml", + data: dummyData, + }}, expectedFileNames: []string{ ".my-ci-folder/test.yml", }, @@ -106,9 +155,10 @@ func TestFetch(t *testing.T) { { name: "Special config - folder", repoConfig: ".my-ci-folder/", - files: []string{ - ".my-ci-folder/test.yml", - }, + files: []file{{ + name: ".my-ci-folder/test.yml", + data: dummyData, + }}, expectedFileNames: []string{ ".my-ci-folder/test.yml", }, @@ -117,9 +167,10 @@ func TestFetch(t *testing.T) { { name: "Special config - subfolder", repoConfig: ".my-ci-folder/my-config/", - files: []string{ - ".my-ci-folder/my-config/test.yml", - }, + files: []file{{ + name: ".my-ci-folder/my-config/test.yml", + data: dummyData, + }}, expectedFileNames: []string{ ".my-ci-folder/my-config/test.yml", }, @@ -128,9 +179,10 @@ func TestFetch(t *testing.T) { { name: "Special config - file", repoConfig: ".config.yml", - files: []string{ - ".config.yml", - }, + files: []file{{ + name: ".config.yml", + data: dummyData, + }}, expectedFileNames: []string{ ".config.yml", }, @@ -139,9 +191,10 @@ func TestFetch(t *testing.T) { { name: "Special config - file inside subfolder", repoConfig: ".my-ci-folder/sub-folder/config.yml", - files: []string{ - ".my-ci-folder/sub-folder/config.yml", - }, + files: []file{{ + name: ".my-ci-folder/sub-folder/config.yml", + data: dummyData, + }}, expectedFileNames: []string{ ".my-ci-folder/sub-folder/config.yml", }, @@ -150,7 +203,7 @@ func TestFetch(t *testing.T) { { name: "Special config - empty repo", repoConfig: ".config.yml", - files: []string{}, + files: []file{}, expectedFileNames: []string{}, expectedError: true, }, @@ -163,12 +216,14 @@ func TestFetch(t *testing.T) { r := new(mocks.Remote) dirs := map[string][]*remote.FileMeta{} for _, file := range tt.files { - r.On("File", mock.Anything, mock.Anything, mock.Anything, file).Return([]byte{}, nil) - path := filepath.Dir(file) - dirs[path] = append(dirs[path], &remote.FileMeta{ - Name: file, - Data: []byte{}, - }) + r.On("File", mock.Anything, mock.Anything, mock.Anything, file.name).Return(file.data, nil) + path := filepath.Dir(file.name) + if path != "." { + dirs[path] = append(dirs[path], &remote.FileMeta{ + Name: file.name, + Data: file.data, + }) + } } for path, files := range dirs { @@ -192,22 +247,11 @@ func TestFetch(t *testing.T) { 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) { - var receivedFileNames []string - for _, file := range files { - receivedFileNames = append(receivedFileNames, file.Name) - } - t.Fatal("expected some other pipeline files", tt.expectedFileNames, receivedFileNames) + matchingFiles := make([]string, len(files)) + for i := range files { + matchingFiles[i] = files[i].Name } + assert.ElementsMatch(t, tt.expectedFileNames, matchingFiles, "expected some other pipeline files") }) } }