From 2edc5fcfb78c5dd7ffd5db55a6e89cb8fc1957ab Mon Sep 17 00:00:00 2001 From: Laszlo Fogas Date: Fri, 19 Jul 2019 09:17:47 +0200 Subject: [PATCH 1/3] Pid ID refactor --- server/procBuilder.go | 11 +++++++---- server/procBuilder_test.go | 38 +++++++++++++++++++++++++++++++++++++- 2 files changed, 44 insertions(+), 5 deletions(-) diff --git a/server/procBuilder.go b/server/procBuilder.go index 5eaadeefa..575c1ca9f 100644 --- a/server/procBuilder.go +++ b/server/procBuilder.go @@ -59,7 +59,9 @@ func (b *procBuilder) Build() ([]*buildItem, error) { sort.Sort(remote.ByName(b.Yamls)) - for j, y := range b.Yamls { + pidSequence := 1 + + for _, y := range b.Yamls { // matrix axes axes, err := matrix.ParseString(string(y.Data)) if err != nil { @@ -69,11 +71,11 @@ func (b *procBuilder) Build() ([]*buildItem, error) { axes = append(axes, matrix.Axis{}) } - for i, axis := range axes { + for _, axis := range axes { proc := &model.Proc{ BuildID: b.Curr.ID, - PID: j + i + 1, - PGID: j + i + 1, + PID: pidSequence, + PGID: pidSequence, State: model.StatusPending, Environ: axis, Name: sanitizePath(y.Name), @@ -123,6 +125,7 @@ func (b *procBuilder) Build() ([]*buildItem, error) { item.Labels = map[string]string{} } items = append(items, item) + pidSequence++ } } diff --git a/server/procBuilder_test.go b/server/procBuilder_test.go index 4452dfaf4..d5f32ae7f 100644 --- a/server/procBuilder_test.go +++ b/server/procBuilder_test.go @@ -201,6 +201,42 @@ pipeline: } } if buildItems[1].Proc.State != model.StatusPending { - t.Fatal("Should not run on dev branch") + t.Fatal("Should run on dev branch") + } +} + +func TestTree(t *testing.T) { + build := &model.Build{} + + 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{Data: []byte(` +pipeline: + build: + image: scratch + yyy: ${DRONE_COMMIT_MESSAGE} +`)}, + }, + } + + _, err := b.Build() + if err != nil { + t.Fatal(err) + } + if len(build.Procs) != 3 { + t.Fatal("Should generate three in total") + } + if build.Procs[1].PPID != 1 { + t.Fatal("Clone step should be a children of the stage") + } + if build.Procs[2].PPID != 1 { + t.Fatal("Build step should be a children of the stage") } } From c303a4d4638697545bb2a50aff2d92c7c357e17e Mon Sep 17 00:00:00 2001 From: Laszlo Fogas Date: Fri, 19 Jul 2019 09:18:40 +0200 Subject: [PATCH 2/3] If there are no steps, it shouldn't yield a proc --- server/procBuilder.go | 7 ++++++- server/procBuilder_test.go | 36 ++++++++++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 1 deletion(-) diff --git a/server/procBuilder.go b/server/procBuilder.go index 575c1ca9f..a78318d90 100644 --- a/server/procBuilder.go +++ b/server/procBuilder.go @@ -80,7 +80,6 @@ func (b *procBuilder) Build() ([]*buildItem, error) { Environ: axis, Name: sanitizePath(y.Name), } - b.Curr.Procs = append(b.Curr.Procs, proc) metadata := metadataFromStruct(b.Repo, b.Curr, b.Last, proc, b.Link) environ := b.environmentVariables(metadata, axis) @@ -113,6 +112,10 @@ func (b *procBuilder) Build() ([]*buildItem, error) { ir := b.toInternalRepresentation(parsed, environ, metadata, proc.ID) + if len(ir.Stages) == 0 { + continue + } + item := &buildItem{ Proc: proc, Config: ir, @@ -124,6 +127,8 @@ func (b *procBuilder) Build() ([]*buildItem, error) { if item.Labels == nil { item.Labels = map[string]string{} } + + b.Curr.Procs = append(b.Curr.Procs, proc) items = append(items, item) pidSequence++ } diff --git a/server/procBuilder_test.go b/server/procBuilder_test.go index d5f32ae7f..1c456cc29 100644 --- a/server/procBuilder_test.go +++ b/server/procBuilder_test.go @@ -205,6 +205,42 @@ pipeline: } } +func TestZeroSteps(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{Data: []byte(` +skip_clone: true +pipeline: + build: + when: + branch: notdev + image: scratch + yyy: ${DRONE_COMMIT_MESSAGE} +`)}, + }, + } + + buildItems, err := b.Build() + if err != nil { + t.Fatal(err) + } + 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 TestTree(t *testing.T) { build := &model.Build{} From d560643b266878ac3400dc39f9fb64cdab8c470c Mon Sep 17 00:00:00 2001 From: Laszlo Fogas Date: Fri, 19 Jul 2019 10:13:02 +0200 Subject: [PATCH 3/3] Calculating build steps early on to cut short cases without any runnable steps --- server/hook.go | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/server/hook.go b/server/hook.go index 0becc690c..0d4e5eac5 100644 --- a/server/hook.go +++ b/server/hook.go @@ -177,6 +177,11 @@ func PostHook(c *gin.Context) { return } + if zeroSteps(*build, remoteYamlConfigs) { + c.String(200, "Step conditions yield zero runnable steps") + return + } + if repo.IsGated { // This feature is not clear to me. Reenabling once better understood build.Status = model.StatusBlocked } @@ -293,6 +298,30 @@ 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 { + b := procBuilder{ + Repo: &model.Repo{}, + Curr: &build, + Last: &model.Build{}, + Netrc: &model.Netrc{}, + Secs: []*model.Secret{}, + Regs: []*model.Registry{}, + Link: "", + Yamls: remoteYamlConfigs, + } + + buildItems, err := b.Build() + if err != nil { + return false + } + if len(buildItems) == 0 { + return true + } + + return false +} + func findOrPersistPipelineConfig(build *model.Build, remoteYamlConfig *remote.FileMeta) (*model.Config, error) { sha := shasum(remoteYamlConfig.Data) conf, err := Config.Storage.Config.ConfigFindIdentical(build.RepoID, sha)