From d87fe977d9adb282fff2257eb327838e188223bb Mon Sep 17 00:00:00 2001 From: Piyush Garg Date: Mon, 2 Sep 2024 13:07:09 +0530 Subject: [PATCH] Redesign PAC Resolver This will redesign resolver to work with multiple edge case scenarios. Previously pac resolver was filtering pipelinerun based on annotations but it was resolving all pipelines in .tekton dir leading to resolving unnecessary pipelines and other issue was it was storing task based on task name, instead of annotation name and version, so different version of task were not used across pipelineruns in .tekton dir Now with new design we are first filtering pipelinerun based on annotations, and then processing all pipelineruns one by one and only resolving pipeline related to these pipelineruns. Also we are now maintaining map of tasks with name and version at event level to not re fetch the task and now inline spec replacement in pipelinerun is done one by one so respective task as mentioned in annotation with name and version is used Also before filtering the pipelineruns, we should make sure that no two pipelineruns exists with same name in .tekton dir Added tests for the three scenarios --- pkg/matcher/annotation_tasks_install.go | 80 +++-- pkg/matcher/annotation_tasks_install_test.go | 307 +++++++++++------- pkg/resolve/remote.go | 202 ++++++++---- pkg/resolve/remote_test.go | 87 +++-- pkg/resolve/resolve.go | 106 ++---- ...peline-with-remote-task-from-pipeline.yaml | 17 + ...ine-with-remote-task-from-pipelinerun.yaml | 18 + ...pipelinerun-annotations-and-tektondir.yaml | 18 + ...n-annotations-and-pipeline-annotation.yaml | 19 ++ ...pipelinerun-annotations-and-tektondir.yaml | 18 + test/gitea_test.go | 56 ++++ .../pipelinerun_same_name_on_pull.yaml | 21 ++ .../pipelinerun_same_name_on_push.yaml | 21 ++ test/testdata/pipeline1_in_tektondir.yaml | 19 ++ test/testdata/pipeline2_in_tektondir.yaml | 19 ++ test/testdata/pipelinerun-with-yq-3.yaml | 58 ++++ test/testdata/pipelinerun-with-yq-4.yaml | 63 ++++ .../pipelinerun1_remote_task_annotations.yaml | 13 + .../pipelinerun2_remote_task_annotations.yaml | 12 + 19 files changed, 810 insertions(+), 344 deletions(-) create mode 100644 pkg/resolve/testdata/remote-pipeline-with-remote-task-from-pipeline.yaml create mode 100644 pkg/resolve/testdata/remote-pipeline-with-remote-task-from-pipelinerun.yaml create mode 100644 pkg/resolve/testdata/skip-fetching-multiple-pipelines-of-the-same-name-from-pipelinerun-annotations-and-tektondir.yaml create mode 100644 pkg/resolve/testdata/skip-fetching-multiple-tasks-of-the-same-name-from-pipelinerun-annotations-and-pipeline-annotation.yaml create mode 100644 pkg/resolve/testdata/skip-fetching-multiple-tasks-of-the-same-name-from-pipelinerun-annotations-and-tektondir.yaml create mode 100644 test/testdata/failures/pipelinerun_same_name_on_pull.yaml create mode 100644 test/testdata/failures/pipelinerun_same_name_on_push.yaml create mode 100644 test/testdata/pipeline1_in_tektondir.yaml create mode 100644 test/testdata/pipeline2_in_tektondir.yaml create mode 100644 test/testdata/pipelinerun-with-yq-3.yaml create mode 100644 test/testdata/pipelinerun-with-yq-4.yaml create mode 100644 test/testdata/pipelinerun1_remote_task_annotations.yaml create mode 100644 test/testdata/pipelinerun2_remote_task_annotations.yaml diff --git a/pkg/matcher/annotation_tasks_install.go b/pkg/matcher/annotation_tasks_install.go index 9640d43af..47be7237c 100644 --- a/pkg/matcher/annotation_tasks_install.go +++ b/pkg/matcher/annotation_tasks_install.go @@ -181,60 +181,54 @@ func grabValuesFromAnnotations(annotations map[string]string, annotationReg stri return ret, nil } -// GetTaskFromAnnotations Get task remotely if they are on Annotations. -func (rt RemoteTasks) GetTaskFromAnnotations(ctx context.Context, annotations map[string]string) ([]*tektonv1.Task, error) { - ret := []*tektonv1.Task{} - tasks, err := grabValuesFromAnnotations(annotations, taskAnnotationsRegexp) +func GrabTasksFromAnnotations(annotations map[string]string) ([]string, error) { + return grabValuesFromAnnotations(annotations, taskAnnotationsRegexp) +} + +func GrabPipelineFromAnnotations(annotations map[string]string) (string, error) { + pipelinesAnnotation, err := grabValuesFromAnnotations(annotations, pipelineAnnotationsRegexp) if err != nil { - return nil, err + return "", err } - for _, v := range tasks { - data, err := rt.getRemote(ctx, v, true, "task") - if err != nil { - return nil, fmt.Errorf("error getting remote task \"%s\": %w", v, err) - } - if data == "" { - return nil, fmt.Errorf("could not get remote task \"%s\": returning empty", v) - } - - task, err := rt.convertTotask(ctx, v, data) - if err != nil { - return nil, err - } - ret = append(ret, task) + if len(pipelinesAnnotation) > 1 { + return "", fmt.Errorf("only one pipeline is allowed on remote resolution, we have received multiple of them: %+v", pipelinesAnnotation) } - return ret, nil + if len(pipelinesAnnotation) == 0 { + return "", nil + } + return pipelinesAnnotation[0], nil } -// GetPipelineFromAnnotations Get pipeline remotely if they are on Annotations -// TODO: merge in a generic between the two. -func (rt RemoteTasks) GetPipelineFromAnnotations(ctx context.Context, annotations map[string]string) (*tektonv1.Pipeline, error) { - ret := []*tektonv1.Pipeline{} - pipelinesAnnotation, err := grabValuesFromAnnotations(annotations, pipelineAnnotationsRegexp) +func (rt RemoteTasks) GetTaskFromAnnotationName(ctx context.Context, name string) (*tektonv1.Task, error) { + data, err := rt.getRemote(ctx, name, true, "task") + if err != nil { + return nil, fmt.Errorf("error getting remote task \"%s\": %w", name, err) + } + if data == "" { + return nil, fmt.Errorf("could not get remote task \"%s\": returning empty", name) + } + + task, err := rt.convertTotask(ctx, name, data) if err != nil { return nil, err } - if len(pipelinesAnnotation) > 1 { - return nil, fmt.Errorf("only one pipeline is allowed on remote resolution, we have received multiple of them: %+v", pipelinesAnnotation) + return task, nil +} + +func (rt RemoteTasks) GetPipelineFromAnnotationName(ctx context.Context, name string) (*tektonv1.Pipeline, error) { + data, err := rt.getRemote(ctx, name, true, "pipeline") + if err != nil { + return nil, fmt.Errorf("error getting remote pipeline \"%s\": %w", name, err) } - if len(pipelinesAnnotation) == 0 { - return nil, nil + if data == "" { + return nil, fmt.Errorf("could not get remote pipeline \"%s\": returning empty", name) } - for _, v := range pipelinesAnnotation { - data, err := rt.getRemote(ctx, v, true, "pipeline") - if err != nil { - return nil, fmt.Errorf("error getting remote pipeline %s: %w", v, err) - } - if data == "" { - return nil, fmt.Errorf("could not get remote pipeline \"%s\": returning empty", v) - } - pipeline, err := rt.convertToPipeline(ctx, v, data) - if err != nil { - return nil, err - } - ret = append(ret, pipeline) + + pipeline, err := rt.convertToPipeline(ctx, name, data) + if err != nil { + return nil, err } - return ret[0], nil + return pipeline, nil } // getFileFromLocalFS get task locally if file exist diff --git a/pkg/matcher/annotation_tasks_install_test.go b/pkg/matcher/annotation_tasks_install_test.go index 1e395cf03..77ad07088 100644 --- a/pkg/matcher/annotation_tasks_install_test.go +++ b/pkg/matcher/annotation_tasks_install_test.go @@ -15,6 +15,7 @@ import ( "github.com/openshift-pipelines/pipelines-as-code/pkg/params/settings" httptesthelper "github.com/openshift-pipelines/pipelines-as-code/pkg/test/http" "github.com/openshift-pipelines/pipelines-as-code/pkg/test/provider" + testifyassert "github.com/stretchr/testify/assert" tektonv1beta1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1" "go.uber.org/zap" zapobserver "go.uber.org/zap/zaptest/observer" @@ -46,7 +47,138 @@ func readTDfile(t *testing.T, testname string) string { return string(data) } -func TestRemoteTasksGetTaskFromAnnotations(t *testing.T) { +func TestGrabTasksFromAnnotation(t *testing.T) { + tests := []struct { + annotations map[string]string + expected []string + name string + wantErr string + }{ + { + name: "single task", + annotations: map[string]string{ + keys.Task: "[http://remote.task]", + }, + expected: []string{"http://remote.task"}, + }, + { + name: "wrong key", + annotations: map[string]string{ + keys.Task: "[http://remote.task]", + pipelinesascode.GroupName + "/taskA": "[http://other.task]", // That's wrong this would be skipped + }, + expected: []string{"http://remote.task"}, + }, + { + name: "multiple tasks", + annotations: map[string]string{ + keys.Task: "[http://remote.task]", + keys.Task + "-1": "[http://other.task]", + }, + expected: []string{"http://other.task", "http://remote.task"}, + }, + { + name: "multiple tasks with random order", + annotations: map[string]string{ + keys.Task: "[http://remote.task]", + keys.Task + "-5": "[http://other.task]", + }, + expected: []string{"http://other.task", "http://remote.task"}, + }, + { + name: "multiple tasks with only orders", + annotations: map[string]string{ + keys.Task + "-5": "[http://remote.task]", + keys.Task + "-1": "[http://other.task]", + }, + expected: []string{"http://other.task", "http://remote.task"}, + }, + { + name: "multiple tasks with one annotation", + annotations: map[string]string{ + keys.Task + "-1": "[http://other.task, http://remote.task]", + }, + expected: []string{"http://other.task", "http://remote.task"}, + }, + { + name: "test-annotations-remote-http-bad-annotation", + annotations: map[string]string{ + keys.Task: "[http://remote.task", + }, + expected: []string{}, + wantErr: "annotations in pipeline are in wrong format: [http://remote.task", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + output, err := GrabTasksFromAnnotations(tt.annotations) + testifyassert.ElementsMatch(t, tt.expected, output) + if tt.wantErr != "" { + assert.ErrorContains(t, err, tt.wantErr, "We should have get an error with %v but we didn't", tt.wantErr) + } + }) + } +} + +func TestGrabPipelineFromAnnotation(t *testing.T) { + tests := []struct { + annotations map[string]string + expected string + name string + wantErr string + }{ + { + name: "single pipeline", + annotations: map[string]string{ + keys.Pipeline: "[http://remote.task]", + }, + expected: "http://remote.task", + }, + { + name: "sing pipeline and a wrong key", + annotations: map[string]string{ + keys.Pipeline: "[http://remote.task]", + pipelinesascode.GroupName + "/pipelineA": "[http://other.task]", // That's wrong this would be skipped + }, + expected: "http://remote.task", + }, + { + name: "single pipeline with only wrong key", + annotations: map[string]string{ + keys.Pipeline + "-1": "[http://other.task]", + }, + expected: "", + }, + { + name: "multiple pipelines with one annotation", + annotations: map[string]string{ + keys.Pipeline: "[http://other.task, http://remote.task]", + }, + expected: "", + wantErr: "only one pipeline is allowed on remote resolution, we have received multiple of them: [http://other.task http://remote.task]", + }, + { + name: "test-annotations-remote-http-bad-annotation", + annotations: map[string]string{ + keys.Pipeline: "[http://remote.task", + }, + expected: "", + wantErr: "annotations in pipeline are in wrong format: [http://remote.task", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + output, err := GrabPipelineFromAnnotations(tt.annotations) + assert.Equal(t, tt.expected, output) + if tt.wantErr != "" { + assert.ErrorContains(t, err, tt.wantErr, "We should have get an error with %v but we didn't", tt.wantErr) + return + } + }) + } +} + +func TestGetTaskFromAnnotationName(t *testing.T) { var hubCatalogs sync.Map hubCatalogs.Store( "default", settings.HubCatalog{ @@ -61,7 +193,7 @@ func TestRemoteTasksGetTaskFromAnnotations(t *testing.T) { Name: testCatalogHubName, }) tests := []struct { - annotations map[string]string + task string filesInsideRepo map[string]string gotTaskName string name string @@ -73,9 +205,7 @@ func TestRemoteTasksGetTaskFromAnnotations(t *testing.T) { }{ { name: "test-annotations-error-remote-http-not-k8", - annotations: map[string]string{ - keys.Task: "[http://remote.task]", - }, + task: "http://remote.task", remoteURLS: map[string]map[string]string{ "http://remote.task": { "body": "", @@ -85,26 +215,20 @@ func TestRemoteTasksGetTaskFromAnnotations(t *testing.T) { wantErr: "returning empty", }, { - name: "test-good-coming-from-provider", - annotations: map[string]string{ - keys.Task: "http://provider/remote.task", - }, + name: "test-good-coming-from-provider", + task: "http://provider/remote.task", wantProviderRemoteTask: true, wantErr: "returning empty", }, { - name: "test-bad-coming-from-provider", - annotations: map[string]string{ - keys.Task: "http://provider/remote.task", - }, + name: "test-bad-coming-from-provider", + task: "http://provider/remote.task", wantProviderRemoteTask: false, wantErr: "error getting remote task", }, { name: "test-annotations-remote-http", - annotations: map[string]string{ - keys.Task: "[http://remote.task]", - }, + task: "http://remote.task", remoteURLS: map[string]map[string]string{ "http://remote.task": { "body": readTDfile(t, "task-good"), @@ -128,10 +252,8 @@ func TestRemoteTasksGetTaskFromAnnotations(t *testing.T) { // wantErr: "cannot be validated properly", // }, { - name: "test-annotations-remote-https", - annotations: map[string]string{ - keys.Task: "[https://remote.task]", - }, + name: "test-annotations-remote-https", + task: "https://remote.task", gotTaskName: "task", remoteURLS: map[string]map[string]string{ "https://remote.task": { @@ -141,72 +263,52 @@ func TestRemoteTasksGetTaskFromAnnotations(t *testing.T) { }, }, { - name: "test-annotations-inside-repo", - annotations: map[string]string{ - keys.Task: "[be/healthy]", - }, - gotTaskName: "task", - filesInsideRepo: map[string]string{ - "be/healthy": readTDfile(t, "task-good"), - }, - runevent: info.Event{ - SHA: "007", - }, - }, - { - name: "test-annotations-remote-http-skipping-notmatching", - annotations: map[string]string{ - keys.Task: "[http://remote.task]", - pipelinesascode.GroupName + "/taskA": "[http://other.task]", // That's wrong this would be skipped - }, - gotTaskName: "task", + name: "bad/not a tasl", + task: "http://remote.task", remoteURLS: map[string]map[string]string{ "http://remote.task": { - "body": readTDfile(t, "task-good"), + "body": readTDfile(t, "pipeline-good"), "code": "200", }, }, + wantErr: "remote task from uri: http://remote.task has not been recognized as a tekton task", }, { - name: "test-annotations-remote-http-bad-annotation", - annotations: map[string]string{ - keys.Task: "[http://remote.task", + name: "test-annotations-inside-repo", + task: "be/healthy", + gotTaskName: "task", + filesInsideRepo: map[string]string{ + "be/healthy": readTDfile(t, "task-good"), + }, + runevent: info.Event{ + SHA: "007", }, - wantErr: "annotations in pipeline are in wrong format", }, { - name: "test-annotations-remote-inside-file-not-found", - annotations: map[string]string{ - keys.Task: "[pas/la]", - }, + name: "test-annotations-remote-inside-file-not-found", + task: "pas/la", wantErr: "could not find", runevent: info.Event{ SHA: "007", }, }, { - name: "test-annotations-remote-no-event-not-found-no-error", - annotations: map[string]string{ - keys.Task: "[not/here]", - }, + name: "test-annotations-remote-no-event-not-found-no-error", + task: "not/here", wantLog: "could not find remote file not/here", wantErr: "returning empty", }, { - name: "test-annotations-unknown-hub", - annotations: map[string]string{ - keys.Task: "[foo://bar]", - }, + name: "test-annotations-unknown-hub", + task: "foo://bar", wantLog: "custom catalog foo is not found", wantErr: "could not get remote task \"foo://bar\": returning empty", }, { name: "test-get-from-custom-hub", gotTaskName: "task", - annotations: map[string]string{ - keys.Task: "[anotherHub://chmouzie]", - }, - wantLog: "successfully fetched task chmouzie from custom catalog HUB anotherHub on URL https://mybelovedhub", + task: "anotherHub://chmouzie", + wantLog: "successfully fetched task chmouzie from custom catalog HUB anotherHub on URL https://mybelovedhub", remoteURLS: map[string]map[string]string{ testHubURL + "/resource/" + testCatalogHubName + "/task/chmouzie": { "body": `{"data": {"LatestVersion": {"version": "0.1"}}}`, @@ -221,9 +323,7 @@ func TestRemoteTasksGetTaskFromAnnotations(t *testing.T) { { name: "test-get-from-hub-latest", gotTaskName: "task", - annotations: map[string]string{ - keys.Task: "[chmouzie]", - }, + task: "chmouzie", remoteURLS: map[string]map[string]string{ testHubURL + "/resource/" + testCatalogHubName + "/task/chmouzie": { "body": `{"data": {"LatestVersion": {"version": "0.1"}}}`, @@ -238,9 +338,7 @@ func TestRemoteTasksGetTaskFromAnnotations(t *testing.T) { { name: "test-get-from-hub-specific-version", gotTaskName: "task", - annotations: map[string]string{ - keys.Task: "[chmouzie:0.2]", - }, + task: "chmouzie:0.2", remoteURLS: map[string]map[string]string{ testHubURL + "/resource/" + testCatalogHubName + "/task/chmouzie/0.2": { "body": `{}`, @@ -282,7 +380,7 @@ func TestRemoteTasksGetTaskFromAnnotations(t *testing.T) { Event: &tt.runevent, } - got, err := rt.GetTaskFromAnnotations(ctx, tt.annotations) + got, err := rt.GetTaskFromAnnotationName(ctx, tt.task) if tt.wantLog != "" { assert.Assert(t, len(fakelog.FilterMessageSnippet(tt.wantLog).TakeAll()) > 0, "could not find log message: got ", fakelog) } @@ -291,16 +389,16 @@ func TestRemoteTasksGetTaskFromAnnotations(t *testing.T) { return } assert.NilError(t, err, "GetTaskFromAnnotations() error = %v, wantErr %v", err, tt.wantErr) - assert.Assert(t, len(got) > 0, "GetTaskFromAnnotations() error no tasks has been processed") + assert.Assert(t, got != nil, "GetTaskFromAnnotations() error no tasks has been processed") if tt.gotTaskName != "" { - assert.Equal(t, tt.gotTaskName, got[0].GetName()) + assert.Equal(t, tt.gotTaskName, got.GetName()) } }) } } -func TestGetPipelineFromAnnotations(t *testing.T) { +func TestGetPipelineFromAnnotationName(t *testing.T) { var hubCatalogs sync.Map hubCatalogs.Store( "default", settings.HubCatalog{ @@ -315,7 +413,7 @@ func TestGetPipelineFromAnnotations(t *testing.T) { Name: testCatalogHubName, }) tests := []struct { - annotations map[string]string + pipeline string filesInsideRepo map[string]string gotPipelineName string name string @@ -327,9 +425,7 @@ func TestGetPipelineFromAnnotations(t *testing.T) { { name: "good/fetching from remote http", gotPipelineName: "pipeline", - annotations: map[string]string{ - keys.Pipeline: "[http://remote.pipeline]", - }, + pipeline: "http://remote.pipeline", remoteURLS: map[string]map[string]string{ "http://remote.pipeline": { "body": readTDfile(t, "pipeline-good"), @@ -340,9 +436,7 @@ func TestGetPipelineFromAnnotations(t *testing.T) { { name: "good/fetching with bundle", gotPipelineName: "pipeline", - annotations: map[string]string{ - keys.Pipeline: "[http://remote.pipeline]", - }, + pipeline: "http://remote.pipeline", remoteURLS: map[string]map[string]string{ "http://remote.pipeline": { "body": readTDfile(t, "pipeline-good-bundle"), @@ -378,10 +472,8 @@ func TestGetPipelineFromAnnotations(t *testing.T) { // wantErr: "emote pipeline from uri: http://remote.pipeline with name pipeline cannot be validated: expected at least one, got none:", // }, { - name: "bad/error getting pipeline", - annotations: map[string]string{ - keys.Pipeline: "[http://remote.pipeline]", - }, + name: "bad/error getting pipeline", + pipeline: "http://remote.pipeline", remoteURLS: map[string]map[string]string{ "http://remote.pipeline": { "code": "501", @@ -390,10 +482,8 @@ func TestGetPipelineFromAnnotations(t *testing.T) { wantErr: "error getting remote pipeline", }, { - name: "bad/not a pipeline", - annotations: map[string]string{ - keys.Pipeline: "[http://remote.pipeline]", - }, + name: "bad/not a pipeline", + pipeline: "http://remote.pipeline", remoteURLS: map[string]map[string]string{ "http://remote.pipeline": { "body": readTDfile(t, "task-good"), @@ -403,17 +493,13 @@ func TestGetPipelineFromAnnotations(t *testing.T) { wantErr: "remote pipeline from uri: http://remote.pipeline has not been recognized as a tekton pipeline", }, { - name: "bad/could not get remote", - annotations: map[string]string{ - keys.Pipeline: "[http://nowhere.pipeline]", - }, - wantErr: "error getting remote pipeline", + name: "bad/could not get remote", + pipeline: "http://nowhere.pipeline", + wantErr: "error getting remote pipeline", }, { - name: "bad/returning empty", - annotations: map[string]string{ - keys.Pipeline: "[http://remote.pipeline]", - }, + name: "bad/returning empty", + pipeline: "http://remote.pipeline", remoteURLS: map[string]map[string]string{ "http://remote.pipeline": { "body": "", @@ -423,27 +509,16 @@ func TestGetPipelineFromAnnotations(t *testing.T) { wantErr: "returning empty", }, { - name: "bad/more than one pipeline", - annotations: map[string]string{ - keys.Pipeline: "[http://foo.bar, http://remote.pipeline]", - }, - wantErr: "only one pipeline is allowed on remote", - }, - { - name: "test-annotations-unknown-hub", - annotations: map[string]string{ - keys.Pipeline: "[foo://bar]", - }, - wantLog: "custom catalog foo is not found", - wantErr: "could not get remote pipeline \"foo://bar\": returning empty", + name: "test-annotations-unknown-hub", + pipeline: "foo://bar", + wantLog: "custom catalog foo is not found", + wantErr: "could not get remote pipeline \"foo://bar\": returning empty", }, { name: "test-get-from-custom-hub", gotPipelineName: "pipeline", - annotations: map[string]string{ - keys.Pipeline: "[anotherHub://chmouzie]", - }, - wantLog: "successfully fetched pipeline chmouzie from custom catalog HUB anotherHub on URL https://mybelovedhub", + pipeline: "anotherHub://chmouzie", + wantLog: "successfully fetched pipeline chmouzie from custom catalog HUB anotherHub on URL https://mybelovedhub", remoteURLS: map[string]map[string]string{ testHubURL + "/resource/" + testCatalogHubName + "/pipeline/chmouzie": { "body": `{"data": {"LatestVersion": {"version": "0.1"}}}`, @@ -458,9 +533,7 @@ func TestGetPipelineFromAnnotations(t *testing.T) { { name: "test-get-from-hub-latest", gotPipelineName: "pipeline", - annotations: map[string]string{ - keys.Pipeline: "[chmouzie]", - }, + pipeline: "chmouzie", remoteURLS: map[string]map[string]string{ testHubURL + "/resource/" + testCatalogHubName + "/pipeline/chmouzie": { "body": `{"data": {"LatestVersion": {"version": "0.1"}}}`, @@ -475,9 +548,7 @@ func TestGetPipelineFromAnnotations(t *testing.T) { { name: "test-get-from-hub-specific-version", gotPipelineName: "pipeline", - annotations: map[string]string{ - keys.Pipeline: "[chmouzie:0.2]", - }, + pipeline: "chmouzie:0.2", remoteURLS: map[string]map[string]string{ testHubURL + "/resource/" + testCatalogHubName + "/pipeline/chmouzie/0.2": { "body": `{}`, @@ -519,7 +590,7 @@ func TestGetPipelineFromAnnotations(t *testing.T) { Event: &tt.runevent, } - got, err := rt.GetPipelineFromAnnotations(ctx, tt.annotations) + got, err := rt.GetPipelineFromAnnotationName(ctx, tt.pipeline) if tt.wantErr != "" { assert.ErrorContains(t, err, tt.wantErr, "We should have get an error with %v but we didn't", tt.wantErr) diff --git a/pkg/resolve/remote.go b/pkg/resolve/remote.go index 359dc74e5..501a836a2 100644 --- a/pkg/resolve/remote.go +++ b/pkg/resolve/remote.go @@ -5,22 +5,21 @@ import ( "fmt" "github.com/openshift-pipelines/pipelines-as-code/pkg/matcher" + tektonv1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1" ) type NamedItem interface { GetName() string } -func alreadySeen[T NamedItem](items []T, item T) bool { - for _, value := range items { - if value.GetName() == item.GetName() { - return true - } +func alreadyFetchedResource[T NamedItem](resources map[string]T, resourceName string) bool { + if _, ok := resources[resourceName]; ok { + return true } return false } -// getRemotes will get remote tasks or Pipelines from annotations. +// resolveRemoteResources will get remote tasks or Pipelines from annotations. // // It already has some tasks or pipeline coming from the tekton directory stored in [types] // @@ -32,74 +31,161 @@ func alreadySeen[T NamedItem](items []T, item T) bool { // // The precedence logic for Pipeline is first from PipelineRun annotations and // then from Tekton directory. -func getRemotes(ctx context.Context, rt *matcher.RemoteTasks, types TektonTypes) (TektonTypes, error) { - remoteType := &TektonTypes{} +func resolveRemoteResources(ctx context.Context, rt *matcher.RemoteTasks, types TektonTypes, ropt *Opts) ([]*tektonv1.PipelineRun, error) { + // contain Resources fetched for the event + fetchedResourcesForEvent := FetchedResources{ + Tasks: map[string]*tektonv1.Task{}, + Pipelines: map[string]*tektonv1.Pipeline{}, + } + pipelineRuns := []*tektonv1.PipelineRun{} for _, pipelinerun := range types.PipelineRuns { - if len(pipelinerun.GetObjectMeta().GetAnnotations()) == 0 { - continue - } - - // get first all the tasks from the pipelinerun annotations - remoteTasks, err := rt.GetTaskFromAnnotations(ctx, pipelinerun.GetObjectMeta().GetAnnotations()) - if err != nil { - return TektonTypes{}, fmt.Errorf("error getting remote task from pipelinerun annotations: %w", err) + // contain Resources specific to run + fetchedResourcesForPipelineRun := FetchedResourcesForRun{ + Tasks: map[string]*tektonv1.Task{}, } + var pipeline *tektonv1.Pipeline + var err error + if ropt.RemoteTasks { + // no annotations on run, then skip + if pipelinerun.GetObjectMeta().GetAnnotations() == nil { + continue + } - for _, task := range remoteTasks { - if alreadySeen(remoteType.Tasks, task) { - rt.Logger.Debugf("skipping already fetched task %s in annotations on pipelinerun %s", task.GetName(), pipelinerun.GetName()) + if len(pipelinerun.GetObjectMeta().GetAnnotations()) == 0 { continue } - remoteType.Tasks = append(remoteType.Tasks, task) - } - // get the pipeline from the remote annotation if any - remotePipeline, err := rt.GetPipelineFromAnnotations(ctx, pipelinerun.GetObjectMeta().GetAnnotations()) - if err != nil { - return TektonTypes{}, fmt.Errorf("error getting remote pipeline from pipelinerun annotation: %w", err) - } + // get first all the pipeline from the pipelinerun annotations + remotePipeline, err := matcher.GrabPipelineFromAnnotations(pipelinerun.GetObjectMeta().GetAnnotations()) + if err != nil { + return []*tektonv1.PipelineRun{}, fmt.Errorf("error getting remote pipeline from pipelinerun annotations: %w", err) + } - if remotePipeline != nil { - remoteType.Pipelines = append(remoteType.Pipelines, remotePipeline) + // if we got the pipeline name from annotation, we need to fetch the pipeline + if remotePipeline != "" { + // making sure that the pipeline with same annotation name is not fetched + if alreadyFetchedResource(fetchedResourcesForEvent.Pipelines, remotePipeline) { + rt.Logger.Debugf("skipping already fetched pipeline %s in annotations on pipelinerun %s", remotePipeline, pipelinerun.GetName()) + // already fetched, then just get the pipeline to add to run specific Resources + pipeline = fetchedResourcesForEvent.Pipelines[remotePipeline] + } else { + // seems like a new pipeline, fetch it based on name in annotation + pipeline, err = rt.GetPipelineFromAnnotationName(ctx, remotePipeline) + if err != nil { + return []*tektonv1.PipelineRun{}, fmt.Errorf("error getting remote pipeline from pipelinerun annotations: %w", err) + } + // add the pipeline to the Resources fetched for the Event + fetchedResourcesForEvent.Pipelines[remotePipeline] = pipeline + } + } } - } - - // grab the tasks from the remote pipeline - for _, pipeline := range remoteType.Pipelines { - if pipeline.GetObjectMeta().GetAnnotations() == nil { - continue + pipelineTasks := []string{} + // if run is referring to the pipelineRef and pipeline fetched from annotation have name equal to the pipelineRef + if pipelinerun.Spec.PipelineRef != nil && pipelinerun.Spec.PipelineRef.Resolver == "" { + if pipeline == nil || pipeline.Name != pipelinerun.Spec.PipelineRef.Name { + // if pipeline fetched from annotation is not having same name as PipelineRef, then we need to get a local pipeline if exist by same name + pipeline, err = getPipelineByName(pipelinerun.Spec.PipelineRef.Name, types.Pipelines) + if err != nil { + return []*tektonv1.PipelineRun{}, err + } + } + // add the pipeline to the run specific Resources + fetchedResourcesForPipelineRun.Pipeline = pipeline + // grab the tasks, that we may need to fetch for this pipeline from its annotations + if pipeline.GetObjectMeta().GetAnnotations() != nil { + // get all the tasks from the pipeline annotations + pipelineTasks, err = matcher.GrabTasksFromAnnotations(pipeline.GetObjectMeta().GetAnnotations()) + if err != nil { + return []*tektonv1.PipelineRun{}, fmt.Errorf("error getting remote task from pipeline annotations: %w", err) + } + } } - remoteTasks, err := rt.GetTaskFromAnnotations(ctx, pipeline.GetObjectMeta().GetAnnotations()) - if err != nil { - return TektonTypes{}, fmt.Errorf("error getting remote tasks from remote pipeline %s: %w", pipeline.GetName(), err) + + // now start fetching the tasks + if ropt.RemoteTasks { + // first get all the tasks from the pipelinerun annotations + remoteTasks, err := matcher.GrabTasksFromAnnotations(pipelinerun.GetObjectMeta().GetAnnotations()) + if err != nil { + return []*tektonv1.PipelineRun{}, fmt.Errorf("error getting remote task from pipelinerun annotations: %w", err) + } + + // now fetch all the tasks from pipelinerun and pipeline annotations, giving preference to pipelinerun annotation tasks + for _, remoteTask := range append(remoteTasks, pipelineTasks...) { + var task *tektonv1.Task + // if task is already fetched in the event, then just copy the task + if alreadyFetchedResource(fetchedResourcesForEvent.Tasks, remoteTask) { + rt.Logger.Debugf("skipping already fetched task %s in annotations on pipelinerun %s", remoteTask, pipelinerun.GetName()) + task = fetchedResourcesForEvent.Tasks[remoteTask] + } else { + // get the task from annotation name + task, err = rt.GetTaskFromAnnotationName(ctx, remoteTask) + if err != nil { + return []*tektonv1.PipelineRun{}, fmt.Errorf("error getting remote task from pipelinerun annotations: %w", err) + } + // add the newly fetched tasks to fetchedResourcesForEvent with key annotation value + fetchedResourcesForEvent.Tasks[remoteTask] = task + } + // now checking if run specific resources already contain a task with same name, then don't add it + // this is to give preference to the pipelinerun annotation then pipeline annotation + if !alreadyFetchedResource(fetchedResourcesForPipelineRun.Tasks, task.GetName()) { + rt.Logger.Infof("skipping remote task %s as already fetched task %s for pipelinerun %s", remoteTask, task.GetName(), pipelinerun.GetName()) + fetchedResourcesForPipelineRun.Tasks[task.GetName()] = task + } + } } - for _, remoteTask := range remoteTasks { - if alreadySeen(remoteType.Tasks, remoteTask) { - rt.Logger.Infof("skipping remote task %s from remote pipeline %s as already defined in pipelinerun", remoteTask.GetName(), pipeline.GetName()) + // now add all the tasks in .tekton directory to Tasks, as we add them by default if not found in annotation + // we will skip the ones which exist in run specific resources with same name + for _, task := range types.Tasks { + if alreadyFetchedResource(fetchedResourcesForPipelineRun.Tasks, task.GetName()) { + rt.Logger.Infof("overriding task %s coming from .tekton directory by an annotation task for pipelinerun %s", task.GetName(), pipelinerun.GetName()) continue } - remoteType.Tasks = append(remoteType.Tasks, remoteTask) + fetchedResourcesForPipelineRun.Tasks[task.GetName()] = task } - } - ret := TektonTypes{ - PipelineRuns: types.PipelineRuns, - } - // first get the remote types and then the local ones so remote takes precedence - for _, task := range append(remoteType.Tasks, types.Tasks...) { - if alreadySeen(ret.Tasks, task) { - rt.Logger.Infof("overriding task %s coming from tekton directory by an annotation task on the pipeline or pipelinerun", task.GetName()) - continue + // if PipelineRef is used then, first resolve pipeline and replace all taskRef{Finally/Task} of Pipeline, then put inlinePipeline in PipelineRun + if pipelinerun.Spec.PipelineRef != nil && pipelinerun.Spec.PipelineRef.Resolver == "" { + pipelineResolved := fetchedResourcesForPipelineRun.Pipeline + turns, err := inlineTasks(pipelineResolved.Spec.Tasks, ropt, fetchedResourcesForPipelineRun) + if err != nil { + return nil, err + } + pipelineResolved.Spec.Tasks = turns + + fruns, err := inlineTasks(pipelineResolved.Spec.Finally, ropt, fetchedResourcesForPipelineRun) + if err != nil { + return nil, err + } + pipelineResolved.Spec.Finally = fruns + + pipelinerun.Spec.PipelineRef = nil + pipelinerun.Spec.PipelineSpec = &pipelineResolved.Spec } - ret.Tasks = append(ret.Tasks, task) - } - for _, remotePipeline := range append(remoteType.Pipelines, types.Pipelines...) { - if alreadySeen(ret.Pipelines, remotePipeline) { - rt.Logger.Infof("overriding pipeline %s coming from tekton directory by the annotation pipelinerun", remotePipeline.GetName()) - continue + + // if PipelineSpec is used then, now resolve the PipelineRun by replacing all taskRef{Finally/Task} + if pipelinerun.Spec.PipelineSpec != nil { + turns, err := inlineTasks(pipelinerun.Spec.PipelineSpec.Tasks, ropt, fetchedResourcesForPipelineRun) + if err != nil { + return nil, err + } + pipelinerun.Spec.PipelineSpec.Tasks = turns + + fruns, err := inlineTasks(pipelinerun.Spec.PipelineSpec.Finally, ropt, fetchedResourcesForPipelineRun) + if err != nil { + return nil, err + } + pipelinerun.Spec.PipelineSpec.Finally = fruns + } + + // Add a GenerateName based on the pipeline name and a "-" + // if we already have a GenerateName then just keep it like this + if ropt.GenerateName && pipelinerun.GenerateName == "" { + pipelinerun.GenerateName = pipelinerun.Name + "-" + pipelinerun.Name = "" } - ret.Pipelines = append(ret.Pipelines, remotePipeline) + pipelineRuns = append(pipelineRuns, pipelinerun) } - return ret, nil + // return all resolved PipelineRuns + return pipelineRuns, nil } diff --git a/pkg/resolve/remote_test.go b/pkg/resolve/remote_test.go index 5abe456b1..314997632 100644 --- a/pkg/resolve/remote_test.go +++ b/pkg/resolve/remote_test.go @@ -2,6 +2,7 @@ package resolve import ( "fmt" + "os" "strings" "testing" @@ -63,6 +64,9 @@ func TestRemote(t *testing.T) { pipelineTaskRef := []tektonv1.PipelineTask{ { Name: remoteTaskName, + TaskRef: &tektonv1.TaskRef{ + Name: remoteTaskName, + }, }, } pipelinewithTaskRef := ttkn.MakePipeline(remotePipelineName, pipelineTaskRef, map[string]string{ @@ -80,16 +84,16 @@ func TestRemote(t *testing.T) { assert.NilError(t, err) tests := []struct { - name string - pipelineruns []*tektonv1.PipelineRun - tasks []*tektonv1.Task - pipelines []*tektonv1.Pipeline - wantErrSnippet string - remoteURLS map[string]map[string]string - expectedLogsSnippets []string - expectedTaskSpec tektonv1.TaskSpec - expectedPipelinesFetch int - expectedTaskFetch int + name string + pipelineruns []*tektonv1.PipelineRun + tasks []*tektonv1.Task + pipelines []*tektonv1.Pipeline + wantErrSnippet string + remoteURLS map[string]map[string]string + expectedLogsSnippets []string + expectedTaskSpec tektonv1.TaskSpec + expectedPipelineRun string + noPipelineRun bool }{ { name: "remote pipeline with remote task from pipeline", @@ -123,8 +127,7 @@ func TestRemote(t *testing.T) { fmt.Sprintf("successfully fetched %s from remote https url", remotePipelineURL), fmt.Sprintf("successfully fetched %s from remote https url", remoteTaskURL), }, - expectedPipelinesFetch: 1, - expectedTaskFetch: 1, + expectedPipelineRun: "remote-pipeline-with-remote-task-from-pipeline.yaml", }, { name: "remote pipeline with remote task in pipeline overridden from pipelinerun", @@ -156,11 +159,10 @@ func TestRemote(t *testing.T) { }, }, expectedLogsSnippets: []string{ - fmt.Sprintf("successfully fetched %s from remote https url", taskFromPipelineRunURL), fmt.Sprintf("successfully fetched %s from remote https url", remotePipelineURL), + fmt.Sprintf("successfully fetched %s from remote https url", taskFromPipelineRunURL), }, - expectedPipelinesFetch: 1, - expectedTaskFetch: 1, + expectedPipelineRun: "remote-pipeline-with-remote-task-from-pipelinerun.yaml", }, { name: "remote pipelinerun no annotations", @@ -173,10 +175,11 @@ func TestRemote(t *testing.T) { }, ), }, + noPipelineRun: true, }, { name: "error/remote pipelinerun is 404", - wantErrSnippet: "error getting remote pipeline " + remotePipelineURL, + wantErrSnippet: "error getting remote pipeline \"" + remotePipelineURL + "\"", pipelineruns: []*tektonv1.PipelineRun{ ttkn.MakePR(randomPipelineRunName, map[string]string{ apipac.Pipeline: remotePipelineURL, @@ -190,7 +193,7 @@ func TestRemote(t *testing.T) { }, }, { - name: "skipping/multiple tasks of the same name from pipelinerun annotations and pipeline annotation", + name: "skip fetching multiple tasks of the same name from pipelinerun annotations and pipeline annotation", pipelineruns: []*tektonv1.PipelineRun{ ttkn.MakePR(randomPipelineRunName, map[string]string{ apipac.Pipeline: remotePipelineURL, @@ -216,16 +219,13 @@ func TestRemote(t *testing.T) { }, expectedTaskSpec: taskFromPipelineSpec, expectedLogsSnippets: []string{ - fmt.Sprintf("successfully fetched %s from remote https url", remoteTaskURL), - fmt.Sprintf("successfully fetched %s from remote https url", remoteTaskURL), fmt.Sprintf("successfully fetched %s from remote https url", remotePipelineURL), fmt.Sprintf("successfully fetched %s from remote https url", remoteTaskURL), }, - expectedPipelinesFetch: 1, - expectedTaskFetch: 1, + expectedPipelineRun: "skip-fetching-multiple-tasks-of-the-same-name-from-pipelinerun-annotations-and-pipeline-annotation.yaml", }, { - name: "skipping/multiple tasks of the same name from pipelinerun annotations and tektondir", + name: "skip fetching multiple tasks of the same name from pipelinerun annotations and tektondir", pipelineruns: []*tektonv1.PipelineRun{ ttkn.MakePR(randomPipelineRunName, map[string]string{ apipac.Pipeline: remotePipelineURL, @@ -253,17 +253,15 @@ func TestRemote(t *testing.T) { }, expectedTaskSpec: taskFromPipelineSpec, expectedLogsSnippets: []string{ - fmt.Sprintf("successfully fetched %s from remote https url", remoteTaskURL), fmt.Sprintf("successfully fetched %s from remote https url", remotePipelineURL), fmt.Sprintf("successfully fetched %s from remote https url", remoteTaskURL), - fmt.Sprintf("skipping remote task %s from remote pipeline %s as already defined in pipelinerun", remoteTaskName, remotePipelineName), - fmt.Sprintf("overriding task %s coming from tekton directory by an annotation task on the pipeline or pipelinerun", remoteTaskName), + fmt.Sprintf("skipping remote task %s as already fetched task %s for pipelinerun %s", remoteTaskURL, remoteTaskName, randomPipelineRunName), + fmt.Sprintf("overriding task %s coming from .tekton directory by an annotation task for pipelinerun %s", remoteTaskName, randomPipelineRunName), }, - expectedPipelinesFetch: 1, - expectedTaskFetch: 1, + expectedPipelineRun: "skip-fetching-multiple-tasks-of-the-same-name-from-pipelinerun-annotations-and-tektondir.yaml", }, { - name: "skipping/multiple pipelines of the same name from pipelinerun annotations and tektondir", + name: "skip fetching multiple pipelines of the same name from pipelinerun annotations and tektondir", pipelineruns: []*tektonv1.PipelineRun{ ttkn.MakePR(randomPipelineRunName, map[string]string{ apipac.Pipeline: remotePipelineURL, @@ -291,14 +289,11 @@ func TestRemote(t *testing.T) { }, expectedTaskSpec: taskFromPipelineSpec, expectedLogsSnippets: []string{ - fmt.Sprintf("successfully fetched %s from remote https url", remoteTaskURL), fmt.Sprintf("successfully fetched %s from remote https url", remotePipelineURL), fmt.Sprintf("successfully fetched %s from remote https url", remoteTaskURL), - fmt.Sprintf("skipping remote task %s from remote pipeline %s as already defined in pipelinerun", remoteTaskName, remotePipelineName), - fmt.Sprintf("overriding pipeline %s coming from tekton directory by the annotation pipelinerun", remotePipelineName), + fmt.Sprintf("skipping remote task %s as already fetched task %s for pipelinerun %s", remoteTaskURL, remoteTaskName, randomPipelineRunName), }, - expectedPipelinesFetch: 1, - expectedTaskFetch: 1, + expectedPipelineRun: "skip-fetching-multiple-pipelines-of-the-same-name-from-pipelinerun-annotations-and-tektondir.yaml", }, } for _, tt := range tests { @@ -323,32 +318,28 @@ func TestRemote(t *testing.T) { }, }, } - ret, err := getRemotes(ctx, rt, tktype) + ret, err := resolveRemoteResources(ctx, rt, tktype, &Opts{RemoteTasks: true, GenerateName: true}) if tt.wantErrSnippet != "" { assert.ErrorContains(t, err, tt.wantErrSnippet) return } assert.NilError(t, err) - allPipelinesNames := []string{} - for _, task := range ret.Pipelines { - allPipelinesNames = append(allPipelinesNames, task.GetName()) - } - assert.Equal(t, len(ret.Pipelines), tt.expectedPipelinesFetch, allPipelinesNames) - - allTasksNames := []string{} - for _, task := range ret.Tasks { - allTasksNames = append(allTasksNames, task.GetName()) - } - assert.Equal(t, len(ret.Tasks), tt.expectedTaskFetch, allTasksNames) - for k, snippet := range tt.expectedLogsSnippets { logmsg := log.AllUntimed()[k].Message assert.Assert(t, strings.Contains(logmsg, snippet), "\n on index: %d\n we want: %s\n we got: %s", k, snippet, logmsg) } - if tt.expectedTaskFetch > 0 { - assert.DeepEqual(t, tt.expectedTaskSpec, ret.Tasks[0].Spec) + + if tt.noPipelineRun { + assert.Assert(t, len(ret) == 0, "not expecting any pipelinerun") + return } + expectedData, err := os.ReadFile("testdata/" + tt.expectedPipelineRun) + assert.NilError(t, err) + pipelineRun := &tektonv1.PipelineRun{} + err = yaml.Unmarshal(expectedData, pipelineRun) + assert.NilError(t, err) + assert.DeepEqual(t, pipelineRun, ret[0]) }) } } diff --git a/pkg/resolve/resolve.go b/pkg/resolve/resolve.go index 30cfb903e..7546bc17b 100644 --- a/pkg/resolve/resolve.go +++ b/pkg/resolve/resolve.go @@ -20,6 +20,7 @@ import ( yaml "sigs.k8s.io/yaml/goyaml.v2" ) +// Contains Resources Fetched from tektondir. type TektonTypes struct { PipelineRuns []*tektonv1.PipelineRun Pipelines []*tektonv1.Pipeline @@ -28,6 +29,18 @@ type TektonTypes struct { ValidationErrors map[string]string } +// Contains Fetched Resources for Event, with key equals to annotation value. +type FetchedResources struct { + Tasks map[string]*tektonv1.Task + Pipelines map[string]*tektonv1.Pipeline +} + +// Contains Fetched Resources for Run, with key equals to resource name from metadata.name field. +type FetchedResourcesForRun struct { + Tasks map[string]*tektonv1.Task + Pipeline *tektonv1.Pipeline +} + func NewTektonTypes() TektonTypes { return TektonTypes{ ValidationErrors: map[string]string{}, @@ -57,19 +70,10 @@ func detectAtleastNameOrGenerateNameFromPipelineRun(data string) string { return "unknown" } -// getTaskRunByName returns the taskrun with the given name the first one found +// getPipelineByName returns the Pipeline with the given name the first one found // will be matched. It does not handle conflicts so user has fetched multiple -// taskruns with the same name it will just pick up the first one. -// if the taskrun is not found it returns an error. -func getTaskByName(name string, tasks []*tektonv1.Task) (*tektonv1.Task, error) { - for _, value := range tasks { - if value.Name == name { - return value, nil - } - } - return &tektonv1.Task{}, fmt.Errorf("cannot find referenced task %s. if it's a remote task make sure to add it in the annotations", name) -} - +// pipeline with the same name it will just pick up the first one. +// if the pipeline is not found it returns an error. func getPipelineByName(name string, tasks []*tektonv1.Pipeline) (*tektonv1.Pipeline, error) { for _, value := range tasks { if value.Name == name { @@ -115,7 +119,7 @@ func isTektonAPIVersion(apiVersion string) bool { return strings.HasPrefix(apiVersion, "tekton.dev/") || apiVersion == "" } -func inlineTasks(tasks []tektonv1.PipelineTask, ropt *Opts, types TektonTypes) ([]tektonv1.PipelineTask, error) { +func inlineTasks(tasks []tektonv1.PipelineTask, ropt *Opts, remoteResource FetchedResourcesForRun) ([]tektonv1.PipelineTask, error) { pipelineTasks := []tektonv1.PipelineTask{} for _, task := range tasks { if task.TaskRef != nil && @@ -123,9 +127,9 @@ func inlineTasks(tasks []tektonv1.PipelineTask, ropt *Opts, types TektonTypes) ( isTektonAPIVersion(task.TaskRef.APIVersion) && string(task.TaskRef.Kind) != "ClusterTask" && !skippingTask(task.TaskRef.Name, ropt.SkipInlining) { - taskResolved, err := getTaskByName(task.TaskRef.Name, types.Tasks) - if err != nil { - return nil, err + taskResolved, ok := remoteResource.Tasks[task.TaskRef.Name] + if !ok { + return nil, fmt.Errorf("cannot find referenced task %s. if it's a remote task make sure to add it in the annotations", task.TaskRef.Name) } tmd := tektonv1.PipelineTaskMetadata{ Annotations: taskResolved.GetObjectMeta().GetAnnotations(), @@ -209,70 +213,18 @@ func Resolve(ctx context.Context, cs *params.Run, logger *zap.SugaredLogger, pro return []*tektonv1.PipelineRun{}, err } - // Resolve remote annotations on remote task or remote pipeline or tasks - // inside remote pipeline - if ropt.RemoteTasks { - rt := &matcher.RemoteTasks{ - Run: cs, - Event: event, - ProviderInterface: providerintf, - Logger: logger, - } - var err error - if types, err = getRemotes(ctx, rt, types); err != nil { - return []*tektonv1.PipelineRun{}, err - } + rt := &matcher.RemoteTasks{ + Run: cs, + Event: event, + ProviderInterface: providerintf, + Logger: logger, } - // Resolve {Finally/Task}Ref inside Pipeline - for _, pipeline := range types.Pipelines { - pipelineTasks, err := inlineTasks(pipeline.Spec.Tasks, ropt, types) - if err != nil { - return nil, err - } - pipeline.Spec.Tasks = pipelineTasks - - finallyTasks, err := inlineTasks(pipeline.Spec.Finally, ropt, types) - if err != nil { - return nil, err - } - pipeline.Spec.Finally = finallyTasks - } - - for _, pipelinerun := range types.PipelineRuns { - // Resolve {Finally/Task}Ref inside PipelineSpec inside PipelineRun - if pipelinerun.Spec.PipelineSpec != nil { - turns, err := inlineTasks(pipelinerun.Spec.PipelineSpec.Tasks, ropt, types) - if err != nil { - return nil, err - } - pipelinerun.Spec.PipelineSpec.Tasks = turns - - fruns, err := inlineTasks(pipelinerun.Spec.PipelineSpec.Finally, ropt, types) - if err != nil { - return nil, err - } - pipelinerun.Spec.PipelineSpec.Finally = fruns - } - - // Resolve PipelineRef inside PipelineRef - if pipelinerun.Spec.PipelineRef != nil && pipelinerun.Spec.PipelineRef.Resolver == "" { - pipelineResolved, err := getPipelineByName(pipelinerun.Spec.PipelineRef.Name, types.Pipelines) - if err != nil { - return []*tektonv1.PipelineRun{}, err - } - pipelinerun.Spec.PipelineRef = nil - pipelinerun.Spec.PipelineSpec = &pipelineResolved.Spec - } - - // Add a GenerateName based on the pipeline name and a "-" - // if we already have a GenerateName then just keep it like this - if ropt.GenerateName && pipelinerun.GenerateName == "" { - pipelinerun.GenerateName = pipelinerun.Name + "-" - pipelinerun.Name = "" - } + fetchedResources, err := resolveRemoteResources(ctx, rt, types, ropt) + if err != nil { + return []*tektonv1.PipelineRun{}, err } - return types.PipelineRuns, nil + return fetchedResources, nil } func MetadataResolve(prs []*tektonv1.PipelineRun) ([]*tektonv1.PipelineRun, error) { diff --git a/pkg/resolve/testdata/remote-pipeline-with-remote-task-from-pipeline.yaml b/pkg/resolve/testdata/remote-pipeline-with-remote-task-from-pipeline.yaml new file mode 100644 index 000000000..84ccf0a6b --- /dev/null +++ b/pkg/resolve/testdata/remote-pipeline-with-remote-task-from-pipeline.yaml @@ -0,0 +1,17 @@ +apiVersion: tekton.dev/v1 +kind: PipelineRun +metadata: + annotations: + pipelinesascode.tekton.dev/pipeline: http://remote/remote-pipeline + generateName: pipelinerun-abc- +spec: + pipelineSpec: + tasks: + - name: remote-task + taskSpec: + steps: + - name: step1 + image: scratch + command: + - "true" + finally: [] diff --git a/pkg/resolve/testdata/remote-pipeline-with-remote-task-from-pipelinerun.yaml b/pkg/resolve/testdata/remote-pipeline-with-remote-task-from-pipelinerun.yaml new file mode 100644 index 000000000..4ebb11c56 --- /dev/null +++ b/pkg/resolve/testdata/remote-pipeline-with-remote-task-from-pipelinerun.yaml @@ -0,0 +1,18 @@ +apiVersion: tekton.dev/v1 +kind: PipelineRun +metadata: + annotations: + pipelinesascode.tekton.dev/pipeline: http://remote/remote-pipeline + pipelinesascode.tekton.dev/task: http://remote/task-from-pipelinerun + generateName: pipelinerun-abc- +spec: + pipelineSpec: + tasks: + - name: remote-task + taskSpec: + steps: + - name: frompipelinerun + image: scratch + command: + - "false" + finally: [] diff --git a/pkg/resolve/testdata/skip-fetching-multiple-pipelines-of-the-same-name-from-pipelinerun-annotations-and-tektondir.yaml b/pkg/resolve/testdata/skip-fetching-multiple-pipelines-of-the-same-name-from-pipelinerun-annotations-and-tektondir.yaml new file mode 100644 index 000000000..91561ba87 --- /dev/null +++ b/pkg/resolve/testdata/skip-fetching-multiple-pipelines-of-the-same-name-from-pipelinerun-annotations-and-tektondir.yaml @@ -0,0 +1,18 @@ +apiVersion: tekton.dev/v1 +kind: PipelineRun +metadata: + annotations: + pipelinesascode.tekton.dev/pipeline: http://remote/remote-pipeline + pipelinesascode.tekton.dev/task: http://remote/remote-task + generateName: pipelinerun-abc- +spec: + pipelineSpec: + tasks: + - name: remote-task + taskSpec: + steps: + - name: step1 + image: scratch + command: + - "true" + finally: [] diff --git a/pkg/resolve/testdata/skip-fetching-multiple-tasks-of-the-same-name-from-pipelinerun-annotations-and-pipeline-annotation.yaml b/pkg/resolve/testdata/skip-fetching-multiple-tasks-of-the-same-name-from-pipelinerun-annotations-and-pipeline-annotation.yaml new file mode 100644 index 000000000..e7b6edd44 --- /dev/null +++ b/pkg/resolve/testdata/skip-fetching-multiple-tasks-of-the-same-name-from-pipelinerun-annotations-and-pipeline-annotation.yaml @@ -0,0 +1,19 @@ +apiVersion: tekton.dev/v1 +kind: PipelineRun +metadata: + annotations: + pipelinesascode.tekton.dev/pipeline: http://remote/remote-pipeline + pipelinesascode.tekton.dev/task: http://remote/remote-task + pipelinesascode.tekton.dev/task-1: http://remote/remote-task + generateName: pipelinerun-abc- +spec: + pipelineSpec: + tasks: + - name: remote-task + taskSpec: + steps: + - name: step1 + image: scratch + command: + - "true" + finally: [] diff --git a/pkg/resolve/testdata/skip-fetching-multiple-tasks-of-the-same-name-from-pipelinerun-annotations-and-tektondir.yaml b/pkg/resolve/testdata/skip-fetching-multiple-tasks-of-the-same-name-from-pipelinerun-annotations-and-tektondir.yaml new file mode 100644 index 000000000..91561ba87 --- /dev/null +++ b/pkg/resolve/testdata/skip-fetching-multiple-tasks-of-the-same-name-from-pipelinerun-annotations-and-tektondir.yaml @@ -0,0 +1,18 @@ +apiVersion: tekton.dev/v1 +kind: PipelineRun +metadata: + annotations: + pipelinesascode.tekton.dev/pipeline: http://remote/remote-pipeline + pipelinesascode.tekton.dev/task: http://remote/remote-task + generateName: pipelinerun-abc- +spec: + pipelineSpec: + tasks: + - name: remote-task + taskSpec: + steps: + - name: step1 + image: scratch + command: + - "true" + finally: [] diff --git a/test/gitea_test.go b/test/gitea_test.go index 6dffd361f..3149f97ac 100644 --- a/test/gitea_test.go +++ b/test/gitea_test.go @@ -65,6 +65,21 @@ func TestGiteaPullRequestTaskAnnotations(t *testing.T) { defer f() } +func TestGiteaPullRequestTaskAnnotationsSameTaskDifferentVersion(t *testing.T) { + topts := &tgitea.TestOpts{ + Regexp: successRegexp, + TargetEvent: triggertype.PullRequest.String(), + YAMLFiles: map[string]string{ + ".tekton/pr1.yaml": "testdata/pipelinerun-with-yq-3.yaml", + ".tekton/pr2.yaml": "testdata/pipelinerun-with-yq-4.yaml", + }, + CheckForNumberStatus: 2, + CheckForStatus: "success", + } + _, f := tgitea.TestPR(t, topts) + defer f() +} + func TestGiteaUseDisplayName(t *testing.T) { topts := &tgitea.TestOpts{ Regexp: regexp.MustCompile(`.*The Task name is Task.*`), @@ -100,6 +115,27 @@ func TestGiteaPullRequestPipelineAnnotations(t *testing.T) { defer f() } +func TestGiteaPullRequestResolvePipelineOnlyAssociatedWithPipelineRunFilterted(t *testing.T) { + topts := &tgitea.TestOpts{ + Regexp: successRegexp, + TargetEvent: triggertype.PullRequest.String(), + YAMLFiles: map[string]string{ + ".tekton/pr1.yaml": "testdata/pipelinerun1_remote_task_annotations.yaml", + ".tekton/pr2.yaml": "testdata/pipelinerun2_remote_task_annotations.yaml", + ".tekton/pipeline1.yaml": "testdata/pipeline1_in_tektondir.yaml", + ".tekton/pipeline2.yaml": "testdata/pipeline2_in_tektondir.yaml", + }, + ExpectEvents: false, + CheckForStatus: "success", + ExtraArgs: map[string]string{ + "RemoteTaskURL": options.RemoteTaskURL, + "RemoteTaskName": options.RemoteTaskName, + }, + } + _, f := tgitea.TestPR(t, topts) + defer f() +} + func TestGiteaPullRequestPrivateRepository(t *testing.T) { topts := &tgitea.TestOpts{ Regexp: successRegexp, @@ -625,6 +661,26 @@ func TestGiteaBadLinkOfTask(t *testing.T) { assert.NilError(t, twait.RegexpMatchingInControllerLog(ctx, topts.ParamsRun, *errre, 10, "controller", github.Int64(20))) } +// TestGiteaPipelineRunWithSameName checks that we fail properly with the error from the +// tekton pipelines controller. We check on the UI interface that we display +// and inside the pac controller. +func TestGiteaPipelineRunWithSameName(t *testing.T) { + topts := &tgitea.TestOpts{ + TargetEvent: triggertype.PullRequest.String(), + YAMLFiles: map[string]string{ + ".tekton/pr1.yaml": "testdata/failures/pipelinerun_same_name_on_pull.yaml", + ".tekton/pr2.yaml": "testdata/failures/pipelinerun_same_name_on_push.yaml", + }, + CheckForStatus: "failure", + ExpectEvents: true, + Regexp: regexp.MustCompile(".*found multiple pipelinerun in .tekton with the same name*"), + } + ctx, f := tgitea.TestPR(t, topts) + defer f() + errre := regexp.MustCompile("found multiple pipelinerun in .tekton with the same name") + assert.NilError(t, twait.RegexpMatchingInControllerLog(ctx, topts.ParamsRun, *errre, 10, "controller", github.Int64(20))) +} + // TestGiteaProvenanceForDefaultBranch tests the provenance feature of the PipelineRun. // It fetches the PipelineRun definition from the default branch of the repository // as configured on the git platform (e.g., main). diff --git a/test/testdata/failures/pipelinerun_same_name_on_pull.yaml b/test/testdata/failures/pipelinerun_same_name_on_pull.yaml new file mode 100644 index 000000000..1d6e381e4 --- /dev/null +++ b/test/testdata/failures/pipelinerun_same_name_on_pull.yaml @@ -0,0 +1,21 @@ +--- +apiVersion: tekton.dev/v1beta1 +kind: PipelineRun +metadata: + name: "pipelinerun-same-name" + annotations: + pipelinesascode.tekton.dev/target-namespace: "\\ .TargetNamespace //" + pipelinesascode.tekton.dev/on-target-branch: "[\\ .TargetBranch //]" + pipelinesascode.tekton.dev/on-event: "[pull_request]" +spec: + pipelineSpec: + tasks: + - name: task + taskSpec: + steps: + - name: task + image: registry.access.redhat.com/ubi9/ubi-micro + script: | + echo "HELLOMOTO" + sleep 30 + exit 0 \ No newline at end of file diff --git a/test/testdata/failures/pipelinerun_same_name_on_push.yaml b/test/testdata/failures/pipelinerun_same_name_on_push.yaml new file mode 100644 index 000000000..10fa8294f --- /dev/null +++ b/test/testdata/failures/pipelinerun_same_name_on_push.yaml @@ -0,0 +1,21 @@ +--- +apiVersion: tekton.dev/v1beta1 +kind: PipelineRun +metadata: + name: "pipelinerun-same-name" + annotations: + pipelinesascode.tekton.dev/target-namespace: "\\ .TargetNamespace //" + pipelinesascode.tekton.dev/on-target-branch: "[\\ .TargetBranch //]" + pipelinesascode.tekton.dev/on-event: "[push]" +spec: + pipelineSpec: + tasks: + - name: task + taskSpec: + steps: + - name: task + image: registry.access.redhat.com/ubi9/ubi-micro + script: | + echo "HELLOMOTO" + sleep 30 + exit 0 \ No newline at end of file diff --git a/test/testdata/pipeline1_in_tektondir.yaml b/test/testdata/pipeline1_in_tektondir.yaml new file mode 100644 index 000000000..6c96948fa --- /dev/null +++ b/test/testdata/pipeline1_in_tektondir.yaml @@ -0,0 +1,19 @@ +--- +apiVersion: tekton.dev/v1beta1 +kind: Pipeline +metadata: + name: pipeline1-in-tekton-dir +spec: + tasks: + - name: task-spec + taskSpec: + steps: + - name: task-spec + image: registry.access.redhat.com/ubi9/ubi-micro + script: | + echo "Hello from taskSpec" + exit 0 + + - name: \\ .RemoteTaskName // + taskRef: + name: \\ .RemoteTaskName // diff --git a/test/testdata/pipeline2_in_tektondir.yaml b/test/testdata/pipeline2_in_tektondir.yaml new file mode 100644 index 000000000..cdd5f335a --- /dev/null +++ b/test/testdata/pipeline2_in_tektondir.yaml @@ -0,0 +1,19 @@ +--- +apiVersion: tekton.dev/v1beta1 +kind: Pipeline +metadata: + name: pipeline2-in-tekton-dir +spec: + tasks: + - name: task-spec + taskSpec: + steps: + - name: task-spec + image: registry.access.redhat.com/ubi9/ubi-micro + script: | + echo "Hello from taskSpec" + exit 0 + + - name: task-no-annotation + taskRef: + name: task-no-annotation diff --git a/test/testdata/pipelinerun-with-yq-3.yaml b/test/testdata/pipelinerun-with-yq-3.yaml new file mode 100644 index 000000000..2a3e4bf7e --- /dev/null +++ b/test/testdata/pipelinerun-with-yq-3.yaml @@ -0,0 +1,58 @@ +apiVersion: tekton.dev/v1beta1 +kind: PipelineRun +metadata: + name: yq-pipeline-run-3 + annotations: + pipelinesascode.tekton.dev/target-namespace: "\\ .TargetNamespace //" + pipelinesascode.tekton.dev/on-target-branch: "[\\ .TargetBranch //]" + pipelinesascode.tekton.dev/on-event: "[\\ .TargetEvent //]" + pipelinesascode.tekton.dev/task: "git-clone" + pipelinesascode.tekton.dev/task-1: "[yq:0.3]" +spec: + pipelineSpec: + params: + - name: new-image-sha + description: example of a value to use + default: "123" + workspaces: + - name: source + tasks: + - name: fetch-repository + taskRef: + name: git-clone + workspaces: + - name: output + workspace: source + params: + - name: url + value: https://github.com/GijsvanDulmen/yq-task-test + - name: revision + value: "main" + - name: subdirectory + value: "" + - name: deleteExisting + value: "true" + - name: yq-replace + taskRef: + name: yq + runAfter: + - fetch-repository + workspaces: + - name: source + workspace: source + params: + - name: files + value: + - "./helm/values.yaml" + - "./helm/values-development.yaml" + - name: expression + value: .image="$(params.new-image-sha)" + workspaces: + - name: source + volumeClaimTemplate: + spec: + accessModes: + - ReadWriteOnce + resources: + requests: + storage: 100Mi diff --git a/test/testdata/pipelinerun-with-yq-4.yaml b/test/testdata/pipelinerun-with-yq-4.yaml new file mode 100644 index 000000000..3539daf03 --- /dev/null +++ b/test/testdata/pipelinerun-with-yq-4.yaml @@ -0,0 +1,63 @@ +apiVersion: tekton.dev/v1beta1 +kind: PipelineRun +metadata: + name: yq-pipeline-run-4 + annotations: + pipelinesascode.tekton.dev/target-namespace: "\\ .TargetNamespace //" + pipelinesascode.tekton.dev/on-target-branch: "[\\ .TargetBranch //]" + pipelinesascode.tekton.dev/on-event: "[\\ .TargetEvent //]" + pipelinesascode.tekton.dev/task: "git-clone" + pipelinesascode.tekton.dev/task-1: "[yq:0.4]" +spec: + pipelineSpec: + params: + - name: new-image-sha + description: example of a value to use + default: "123" + workspaces: + - name: source + tasks: + - name: fetch-repository + taskRef: + name: git-clone + workspaces: + - name: output + workspace: source + params: + - name: url + value: https://github.com/GijsvanDulmen/yq-task-test + - name: revision + value: "main" + - name: subdirectory + value: "" + - name: deleteExisting + value: "true" + - name: yq-replace + taskRef: + name: yq + runAfter: + - fetch-repository + workspaces: + - name: source + workspace: source + params: + - name: SCRIPT + value: | + for var in "./helm/values.yaml" "./helm/values-development.yaml" + do + /usr/bin/yq eval -i '.image="012345my-new-image-sha"' "$var" + done + workspaces: + - name: source + volumeClaimTemplate: + spec: + accessModes: + - ReadWriteOnce + resources: + requests: + storage: 100Mi + taskRunSpecs: + - pipelineTaskName: yq-replace + taskPodTemplate: + securityContext: + runAsUser: 0 diff --git a/test/testdata/pipelinerun1_remote_task_annotations.yaml b/test/testdata/pipelinerun1_remote_task_annotations.yaml new file mode 100644 index 000000000..15333b60d --- /dev/null +++ b/test/testdata/pipelinerun1_remote_task_annotations.yaml @@ -0,0 +1,13 @@ +--- +apiVersion: tekton.dev/v1beta1 +kind: PipelineRun +metadata: + name: "piplinerun1-remote-annotations" + annotations: + pipelinesascode.tekton.dev/target-namespace: "\\ .TargetNamespace //" + pipelinesascode.tekton.dev/on-target-branch: "[\\ .TargetBranch //]" + pipelinesascode.tekton.dev/on-event: "[\\ .TargetEvent //]" + pipelinesascode.tekton.dev/task-1: "[\\ .RemoteTaskURL //]" +spec: + pipelineRef: + name: pipeline1-in-tekton-dir diff --git a/test/testdata/pipelinerun2_remote_task_annotations.yaml b/test/testdata/pipelinerun2_remote_task_annotations.yaml new file mode 100644 index 000000000..c07e2bcc2 --- /dev/null +++ b/test/testdata/pipelinerun2_remote_task_annotations.yaml @@ -0,0 +1,12 @@ +--- +apiVersion: tekton.dev/v1beta1 +kind: PipelineRun +metadata: + name: "piplinerun2-remote-annotations" + annotations: + pipelinesascode.tekton.dev/target-namespace: "\\ .TargetNamespace //" + pipelinesascode.tekton.dev/on-target-branch: "non-existent-branch" + pipelinesascode.tekton.dev/on-event: "[\\ .TargetEvent //]" +spec: + pipelineRef: + name: pipeline2-in-tekton-dir