From ed51df29b3a11c11b7fc5f10e73ba15fbd47cf46 Mon Sep 17 00:00:00 2001 From: Laszlo Fogas Date: Mon, 22 Jul 2019 13:43:18 +0200 Subject: [PATCH 1/3] Factor out sideeffects --- server/build.go | 2 ++ server/hook.go | 8 ++++---- server/procBuilder.go | 13 ++++++++----- server/procBuilder_test.go | 3 ++- 4 files changed, 16 insertions(+), 10 deletions(-) diff --git a/server/build.go b/server/build.go index b94a7e119..65f7e39da 100644 --- a/server/build.go +++ b/server/build.go @@ -344,6 +344,7 @@ func PostApproval(c *gin.Context) { store.UpdateBuild(c, build) return } + build = setBuildStepsOnBuild(b.Curr, buildItems) err = store.FromContext(c).ProcCreate(build.Procs) if err != nil { @@ -558,6 +559,7 @@ func PostBuild(c *gin.Context) { c.JSON(500, build) return } + build = setBuildStepsOnBuild(b.Curr, buildItems) err = store.FromContext(c).ProcCreate(build.Procs) if err != nil { diff --git a/server/hook.go b/server/hook.go index 6fe847d24..89818441d 100644 --- a/server/hook.go +++ b/server/hook.go @@ -177,7 +177,7 @@ func PostHook(c *gin.Context) { return } - if zeroSteps(*build, remoteYamlConfigs) { + if zeroSteps(build, remoteYamlConfigs) { c.String(200, "Step conditions yield zero runnable steps") return } @@ -261,6 +261,7 @@ func PostHook(c *gin.Context) { store.UpdateBuild(c, build) return } + build = setBuildStepsOnBuild(b.Curr, buildItems) err = store.FromContext(c).ProcCreate(build.Procs) if err != nil { @@ -298,11 +299,10 @@ func branchFiltered(build *model.Build, remoteYamlConfigs []*remote.FileMeta) bo return true } -// uses pass by value as procBuilder has side effects on build. Something to be fixed -func zeroSteps(build model.Build, remoteYamlConfigs []*remote.FileMeta) bool { +func zeroSteps(build *model.Build, remoteYamlConfigs []*remote.FileMeta) bool { b := procBuilder{ Repo: &model.Repo{}, - Curr: &build, + Curr: build, Last: &model.Build{}, Netrc: &model.Netrc{}, Secs: []*model.Secret{}, diff --git a/server/procBuilder.go b/server/procBuilder.go index a78318d90..7ef21ef54 100644 --- a/server/procBuilder.go +++ b/server/procBuilder.go @@ -128,14 +128,11 @@ func (b *procBuilder) Build() ([]*buildItem, error) { item.Labels = map[string]string{} } - b.Curr.Procs = append(b.Curr.Procs, proc) items = append(items, item) pidSequence++ } } - setBuildSteps(b.Curr, items) - return items, nil } @@ -214,8 +211,12 @@ func (b *procBuilder) toInternalRepresentation(parsed *yaml.Config, environ map[ ).Compile(parsed) } -func setBuildSteps(build *model.Build, buildItems []*buildItem) { - pcounter := len(buildItems) +func setBuildStepsOnBuild(build *model.Build, buildItems []*buildItem) *model.Build { + for _, item := range buildItems { + build.Procs = append(build.Procs, item.Proc) + } + pcounter := len(build.Procs) + for _, item := range buildItems { for _, stage := range item.Config.Stages { var gid int @@ -239,6 +240,8 @@ func setBuildSteps(build *model.Build, buildItems []*buildItem) { } } } + + return build } // return the metadata from the cli context. diff --git a/server/procBuilder_test.go b/server/procBuilder_test.go index 1c456cc29..ea3925ee9 100644 --- a/server/procBuilder_test.go +++ b/server/procBuilder_test.go @@ -262,7 +262,8 @@ pipeline: }, } - _, err := b.Build() + buildItems, err := b.Build() + build = setBuildStepsOnBuild(build, buildItems) if err != nil { t.Fatal(err) } From 6879bf62cb3a5c5e31d3c47863c49fb238f8f9e7 Mon Sep 17 00:00:00 2001 From: Laszlo Fogas Date: Mon, 22 Jul 2019 14:13:46 +0200 Subject: [PATCH 2/3] Handling zero-step pipelines as multi-pipeline depedencies --- server/procBuilder.go | 35 ++++++++++++++++++++ server/procBuilder_test.go | 66 ++++++++++++++++++++++++++++++++------ 2 files changed, 91 insertions(+), 10 deletions(-) diff --git a/server/procBuilder.go b/server/procBuilder.go index 7ef21ef54..d96343c95 100644 --- a/server/procBuilder.go +++ b/server/procBuilder.go @@ -133,9 +133,44 @@ func (b *procBuilder) Build() ([]*buildItem, error) { } } + items = filterItemsWithMissingDependencies(items) + return items, nil } +func filterItemsWithMissingDependencies(items []*buildItem) []*buildItem { + itemsToRemove := make([]*buildItem, 0) + + for _, item := range items { + for _, dep := range item.DependsOn { + if !containsItemWithName(dep, items) { + itemsToRemove = append(itemsToRemove, item) + } + } + } + + if len(itemsToRemove) > 0 { + filtered := make([]*buildItem, 0) + for _, item := range items { + if !containsItemWithName(item.Proc.Name, itemsToRemove) { + filtered = append(filtered, item) + } + } + return filtered + } + + return items +} + +func containsItemWithName(name string, items []*buildItem) bool { + for _, item := range items { + if name == item.Proc.Name { + return true + } + } + return false +} + func (b *procBuilder) envsubst_(y string, environ map[string]string) (string, error) { return envsubst.Eval(y, func(name string) string { env := environ[name] diff --git a/server/procBuilder_test.go b/server/procBuilder_test.go index ea3925ee9..d5368bb1a 100644 --- a/server/procBuilder_test.go +++ b/server/procBuilder_test.go @@ -70,13 +70,11 @@ func TestMultiPipeline(t *testing.T) { pipeline: xxx: image: scratch - yyy: ${DRONE_COMMIT_MESSAGE} `)}, &remote.FileMeta{Data: []byte(` pipeline: build: image: scratch - yyy: ${DRONE_COMMIT_MESSAGE} `)}, }, } @@ -100,6 +98,16 @@ func TestDependsOn(t *testing.T) { Regs: []*model.Registry{}, Link: "", Yamls: []*remote.FileMeta{ + &remote.FileMeta{Name: "lint", Data: []byte(` +pipeline: + build: + image: scratch +`)}, + &remote.FileMeta{Name: "test", Data: []byte(` +pipeline: + build: + image: scratch +`)}, &remote.FileMeta{Data: []byte(` pipeline: deploy: @@ -108,7 +116,6 @@ pipeline: depends_on: - lint - test - - build `)}, }, } @@ -117,7 +124,7 @@ depends_on: if err != nil { t.Fatal(err) } - if len(buildItems[0].DependsOn) != 3 { + if len(buildItems[0].DependsOn) != 2 { t.Fatal("Should have 3 dependencies") } if buildItems[0].DependsOn[1] != "test" { @@ -173,14 +180,12 @@ func TestBranchFilter(t *testing.T) { pipeline: xxx: image: scratch - yyy: ${DRONE_COMMIT_MESSAGE} branches: master `)}, &remote.FileMeta{Data: []byte(` pipeline: build: image: scratch - yyy: ${DRONE_COMMIT_MESSAGE} `)}, }, } @@ -224,7 +229,6 @@ pipeline: when: branch: notdev image: scratch - yyy: ${DRONE_COMMIT_MESSAGE} `)}, }, } @@ -236,8 +240,51 @@ pipeline: if len(buildItems) != 0 { t.Fatal("Should not generate a build item if there are no steps") } - if len(build.Procs) != 0 { - t.Fatal("Should not generate a build item if there are no steps") +} + +func TestZeroStepsAsMultiPipelineDeps(t *testing.T) { + build := &model.Build{Branch: "dev"} + + b := procBuilder{ + Repo: &model.Repo{}, + Curr: build, + Last: &model.Build{}, + Netrc: &model.Netrc{}, + Secs: []*model.Secret{}, + Regs: []*model.Registry{}, + Link: "", + Yamls: []*remote.FileMeta{ + &remote.FileMeta{Name: "zerostep", Data: []byte(` +skip_clone: true +pipeline: + build: + when: + branch: notdev + image: scratch +`)}, + &remote.FileMeta{Name: "justastep", Data: []byte(` +pipeline: + build: + image: scratch +`)}, + &remote.FileMeta{Name: "shouldbefiltered", Data: []byte(` +pipeline: + build: + image: scratch +depends_on: [ zerostep ] +`)}, + }, + } + + buildItems, err := b.Build() + if err != nil { + t.Fatal(err) + } + if len(buildItems) != 1 { + t.Fatal("Zerostep and the step that depends on it should not generate a build item") + } + if "justastep" != buildItems[0].Proc.Name { + t.Fatal("justastep should have been generated") } } @@ -257,7 +304,6 @@ func TestTree(t *testing.T) { pipeline: build: image: scratch - yyy: ${DRONE_COMMIT_MESSAGE} `)}, }, } From 3bb0f64025b574e1f7ea23256996360b522abd5c Mon Sep 17 00:00:00 2001 From: Laszlo Fogas Date: Mon, 22 Jul 2019 14:29:15 +0200 Subject: [PATCH 3/3] Handling zero-steps as transitive dependencies --- server/procBuilder.go | 3 ++- server/procBuilder_test.go | 52 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 54 insertions(+), 1 deletion(-) diff --git a/server/procBuilder.go b/server/procBuilder.go index d96343c95..a31a2f711 100644 --- a/server/procBuilder.go +++ b/server/procBuilder.go @@ -156,7 +156,8 @@ func filterItemsWithMissingDependencies(items []*buildItem) []*buildItem { filtered = append(filtered, item) } } - return filtered + // Recursive to handle transitive deps + return filterItemsWithMissingDependencies(filtered) } return items diff --git a/server/procBuilder_test.go b/server/procBuilder_test.go index d5368bb1a..904a7204b 100644 --- a/server/procBuilder_test.go +++ b/server/procBuilder_test.go @@ -288,6 +288,58 @@ depends_on: [ zerostep ] } } +func TestZeroStepsAsMultiPipelineTransitiveDeps(t *testing.T) { + build := &model.Build{Branch: "dev"} + + b := procBuilder{ + Repo: &model.Repo{}, + Curr: build, + Last: &model.Build{}, + Netrc: &model.Netrc{}, + Secs: []*model.Secret{}, + Regs: []*model.Registry{}, + Link: "", + Yamls: []*remote.FileMeta{ + &remote.FileMeta{Name: "zerostep", Data: []byte(` +skip_clone: true +pipeline: + build: + when: + branch: notdev + image: scratch +`)}, + &remote.FileMeta{Name: "justastep", Data: []byte(` +pipeline: + build: + image: scratch +`)}, + &remote.FileMeta{Name: "shouldbefiltered", Data: []byte(` +pipeline: + build: + image: scratch +depends_on: [ zerostep ] +`)}, + &remote.FileMeta{Name: "shouldbefilteredtoo", Data: []byte(` +pipeline: + build: + image: scratch +depends_on: [ shouldbefiltered ] +`)}, + }, + } + + buildItems, err := b.Build() + if err != nil { + t.Fatal(err) + } + if len(buildItems) != 1 { + t.Fatal("Zerostep and the step that depends on it, and the one depending on it should not generate a build item") + } + if "justastep" != buildItems[0].Proc.Name { + t.Fatal("justastep should have been generated") + } +} + func TestTree(t *testing.T) { build := &model.Build{}