Skip to content

Commit

Permalink
Refactor to the remote image library and task references
Browse files Browse the repository at this point in the history
This change is part of larger effort to support OCI image references in
task and pipeline refs. This intial change is a refactor of existing
logic for clarity and in preparation of adding the new API fields into
the Tekton objects.

For more details see the design: https://docs.google.com/document/d/1lXF_SvLwl6OqqGy8JbpSXRj4hWJ6CSImlxlIl4V9rnM/edit#

Principle changes:
- Refactor the remote library to operate more like an independent API.
  It allows the user to Get and List objects in a remote image. Decoupling
  the API of fetching and inspecting images allows this to be reused by
  external clients (like `tkn`).
- Create a new taskref library resolver that knows how to resolve local
  references. This mirrors the remote resolver and now establishes an
  ability for the next PR to create a factory function that assigns the
  correct resolver based on the TaskRun. It also allows us to test this
  logic by putting it into a function.
- Likewise, we have created a new pipelineref library for all the same reasons above.

Signed-off-by: Sunil Thaha <sthaha@redhat.com>
  • Loading branch information
sthaha authored and tekton-robot committed Apr 28, 2020
1 parent 52a14b4 commit b435bd9
Show file tree
Hide file tree
Showing 17 changed files with 766 additions and 304 deletions.
27 changes: 12 additions & 15 deletions pkg/reconciler/pipelinerun/pipelinerun.go
Original file line number Diff line number Diff line change
Expand Up @@ -230,17 +230,6 @@ func (c *Reconciler) Reconcile(ctx context.Context, key string) error {
return merr
}

func (c *Reconciler) getPipelineFunc(tr *v1alpha1.PipelineRun) resources.GetPipeline {
var gtFunc resources.GetPipeline = func(name string) (v1alpha1.PipelineInterface, error) {
p, err := c.pipelineLister.Pipelines(tr.Namespace).Get(name)
if err != nil {
return nil, err
}
return p, nil
}
return gtFunc
}

func (c *Reconciler) updatePipelineResults(ctx context.Context, pr *v1alpha1.PipelineRun) {
if err := pr.ConvertTo(ctx, &v1beta1.PipelineRun{}); err != nil {
if ce, ok := err.(*v1beta1.CannotConvertError); ok {
Expand All @@ -249,8 +238,12 @@ func (c *Reconciler) updatePipelineResults(ctx context.Context, pr *v1alpha1.Pip
return
}

getPipelineFunc := c.getPipelineFunc(pr)
pipelineMeta, pipelineSpec, err := resources.GetPipelineData(ctx, pr, getPipelineFunc)
// TODO: Use factory func instead of hard-coding this once OCI images are supported.
resolver := &resources.LocalPipelineRefResolver{
Namespace: pr.Namespace,
Tektonclient: c.PipelineClientSet,
}
pipelineMeta, pipelineSpec, err := resources.GetPipelineData(ctx, pr, resolver.GetPipeline)
if err != nil {
if ce, ok := err.(*v1beta1.CannotConvertError); ok {
pr.Status.MarkResourceNotConvertible(ce)
Expand Down Expand Up @@ -341,8 +334,12 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1alpha1.PipelineRun) er
return err
}

getPipelineFunc := c.getPipelineFunc(pr)
pipelineMeta, pipelineSpec, err := resources.GetPipelineData(ctx, pr, getPipelineFunc)
// TODO: Use factory func instead of hard-coding this once OCI images are supported.
resolver := &resources.LocalPipelineRefResolver{
Namespace: pr.Namespace,
Tektonclient: c.PipelineClientSet,
}
pipelineMeta, pipelineSpec, err := resources.GetPipelineData(ctx, pr, resolver.GetPipeline)
if err != nil {
if ce, ok := err.(*v1beta1.CannotConvertError); ok {
pr.Status.MarkResourceNotConvertible(ce)
Expand Down
23 changes: 12 additions & 11 deletions pkg/reconciler/pipelinerun/pipelinerun_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,7 @@ func TestReconcile(t *testing.T) {
}

// Check that the expected TaskRun was created
actual := clients.Pipeline.Actions()[0].(ktesting.CreateAction).GetObject()
actual := clients.Pipeline.Actions()[1].(ktesting.CreateAction).GetObject()
expectedTaskRun := tb.TaskRun("test-pipeline-run-success-unit-test-1-mz4c7",
tb.TaskRunNamespace("foo"),
tb.TaskRunOwnerReference("PipelineRun", "test-pipeline-run-success",
Expand Down Expand Up @@ -340,7 +340,7 @@ func TestReconcile_PipelineSpecTaskSpec(t *testing.T) {
}

// Check that the expected TaskRun was created
actual := clients.Pipeline.Actions()[0].(ktesting.CreateAction).GetObject()
actual := clients.Pipeline.Actions()[1].(ktesting.CreateAction).GetObject()
expectedTaskRun := tb.TaskRun("test-pipeline-run-success-unit-test-task-spec-9l9zj",
tb.TaskRunNamespace("foo"),
tb.TaskRunOwnerReference("PipelineRun", "test-pipeline-run-success",
Expand Down Expand Up @@ -812,19 +812,20 @@ func TestReconcileOnCompletedPipelineRun(t *testing.T) {
t.Fatalf("Error reconciling: %s", err)
}

if len(clients.Pipeline.Actions()) != 1 {
if len(clients.Pipeline.Actions()) != 2 {
t.Fatalf("Expected client to have updated the TaskRun status for a completed PipelineRun, but it did not")
}

actual := clients.Pipeline.Actions()[0].(ktesting.UpdateAction).GetObject().(*v1alpha1.PipelineRun)
actual := clients.Pipeline.Actions()[1].(ktesting.UpdateAction).GetObject().(*v1alpha1.PipelineRun)
if actual == nil {
t.Errorf("Expected a PipelineRun to be updated, but it wasn't.")
}
t.Log(clients.Pipeline.Actions())
actions := clients.Pipeline.Actions()
for _, action := range actions {
if action != nil {
resource := action.GetResource().Resource
if resource != "pipelineruns" {
if resource == "taskruns" {
t.Fatalf("Expected client to not have created a TaskRun for the completed PipelineRun, but it did")
}
}
Expand Down Expand Up @@ -959,7 +960,7 @@ func TestReconcileWithTimeout(t *testing.T) {
}

// Check that the expected TaskRun was created
actual := clients.Pipeline.Actions()[0].(ktesting.CreateAction).GetObject().(*v1alpha1.TaskRun)
actual := clients.Pipeline.Actions()[1].(ktesting.CreateAction).GetObject().(*v1alpha1.TaskRun)
if actual == nil {
t.Fatalf("Expected a TaskRun to be created, but it wasn't.")
}
Expand Down Expand Up @@ -1121,7 +1122,7 @@ func TestReconcilePropagateLabels(t *testing.T) {
}

// Check that the expected TaskRun was created
actual := clients.Pipeline.Actions()[0].(ktesting.CreateAction).GetObject().(*v1alpha1.TaskRun)
actual := clients.Pipeline.Actions()[1].(ktesting.CreateAction).GetObject().(*v1alpha1.TaskRun)
if actual == nil {
t.Errorf("Expected a TaskRun to be created, but it wasn't.")
}
Expand Down Expand Up @@ -1352,7 +1353,7 @@ func TestReconcilePropagateAnnotations(t *testing.T) {
}

// Check that the expected TaskRun was created
actual := clients.Pipeline.Actions()[0].(ktesting.CreateAction).GetObject().(*v1alpha1.TaskRun)
actual := clients.Pipeline.Actions()[1].(ktesting.CreateAction).GetObject().(*v1alpha1.TaskRun)
if actual == nil {
t.Errorf("Expected a TaskRun to be created, but it wasn't.")
}
Expand Down Expand Up @@ -1540,8 +1541,8 @@ func TestReconcileWithConditionChecks(t *testing.T) {
}

// Check that the expected TaskRun was created
condCheck0 := clients.Pipeline.Actions()[0].(ktesting.CreateAction).GetObject().(*v1alpha1.TaskRun)
condCheck1 := clients.Pipeline.Actions()[1].(ktesting.CreateAction).GetObject().(*v1alpha1.TaskRun)
condCheck0 := clients.Pipeline.Actions()[1].(ktesting.CreateAction).GetObject().(*v1alpha1.TaskRun)
condCheck1 := clients.Pipeline.Actions()[2].(ktesting.CreateAction).GetObject().(*v1alpha1.TaskRun)
if condCheck0 == nil || condCheck1 == nil {
t.Errorf("Expected two ConditionCheck TaskRuns to be created, but it wasn't.")
}
Expand Down Expand Up @@ -1646,7 +1647,7 @@ func TestReconcileWithFailingConditionChecks(t *testing.T) {
}

// Check that the expected TaskRun was created
actual := clients.Pipeline.Actions()[0].(ktesting.CreateAction).GetObject().(*v1alpha1.TaskRun)
actual := clients.Pipeline.Actions()[1].(ktesting.CreateAction).GetObject().(*v1alpha1.TaskRun)
if actual == nil {
t.Errorf("Expected a ConditionCheck TaskRun to be created, but it wasn't.")
}
Expand Down
41 changes: 41 additions & 0 deletions pkg/reconciler/pipelinerun/resources/pipelineref.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
/*
Copyright 2020 The Tekton Authors
Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package resources

import (
"fmt"

"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1"
clientset "github.com/tektoncd/pipeline/pkg/client/clientset/versioned"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
)

// LocalPipelineRefResolver uses the current cluster to resolve a pipeline reference.
type LocalPipelineRefResolver struct {
Namespace string
Tektonclient clientset.Interface
}

// GetPipeline will resolve a Pipeline from the local cluster using a versioned Tekton client. It will
// return an error if it can't find an appropriate Pipeline for any reason.
func (l *LocalPipelineRefResolver) GetPipeline(name string) (v1alpha1.PipelineInterface, error) {
// If we are going to resolve this reference locally, we need a namespace scope.
if l.Namespace == "" {
return nil, fmt.Errorf("Must specify namespace to resolve reference to pipeline %s", name)
}
return l.Tektonclient.TektonV1alpha1().Pipelines(l.Namespace).Get(name, metav1.GetOptions{})
}
82 changes: 82 additions & 0 deletions pkg/reconciler/pipelinerun/resources/pipelineref_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
/*
Copyright 2020 The Tekton Authors
Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package resources_test

import (
"testing"

"github.com/google/go-cmp/cmp"
"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1"
"github.com/tektoncd/pipeline/pkg/client/clientset/versioned/fake"
"github.com/tektoncd/pipeline/pkg/reconciler/pipelinerun/resources"
tb "github.com/tektoncd/pipeline/test/builder"
"k8s.io/apimachinery/pkg/runtime"
)

func TestPipelineRef(t *testing.T) {
testcases := []struct {
name string
pipelines []runtime.Object
ref *v1alpha1.PipelineRef
expected runtime.Object
wantErr bool
}{
{
name: "local-pipeline",
pipelines: []runtime.Object{
tb.Pipeline("simple", tb.PipelineNamespace("default")),
tb.Pipeline("dummy", tb.PipelineNamespace("default")),
},
ref: &v1alpha1.PipelineRef{
Name: "simple",
},
expected: tb.Pipeline("simple", tb.PipelineNamespace("default")),
wantErr: false,
},
{
name: "pipeline-not-found",
pipelines: []runtime.Object{},
ref: &v1alpha1.PipelineRef{
Name: "simple",
},
expected: nil,
wantErr: true,
},
}

for _, tc := range testcases {
t.Run(tc.name, func(t *testing.T) {
tektonclient := fake.NewSimpleClientset(tc.pipelines...)

lc := &resources.LocalPipelineRefResolver{
Namespace: "default",
Tektonclient: tektonclient,
}

task, err := lc.GetPipeline(tc.ref.Name)
if tc.wantErr && err == nil {
t.Fatal("Expected error but found nil instead")
} else if !tc.wantErr && err != nil {
t.Fatalf("Received unexpected error ( %#v )", err)
}

if diff := cmp.Diff(task, tc.expected); tc.expected != nil && diff != "" {
t.Error(diff)
}
})
}
}
50 changes: 50 additions & 0 deletions pkg/reconciler/taskrun/resources/taskref.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
/*
Copyright 2020 The Tekton Authors
Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package resources

import (
"fmt"

"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1"
clientset "github.com/tektoncd/pipeline/pkg/client/clientset/versioned"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
)

// LocalTaskRefResolver uses the current cluster to resolve a task reference.
type LocalTaskRefResolver struct {
Namespace string
Kind v1alpha1.TaskKind
Tektonclient clientset.Interface
}

// GetTask will resolve either a Task or ClusterTask from the local cluster using a versioned Tekton client. It will
// return an error if it can't find an appropriate Task for any reason.
func (l *LocalTaskRefResolver) GetTask(name string) (v1alpha1.TaskInterface, error) {
if l.Kind == v1alpha1.ClusterTaskKind {
task, err := l.Tektonclient.TektonV1alpha1().ClusterTasks().Get(name, metav1.GetOptions{})
if err != nil {
return nil, err
}
return task, nil
}

// If we are going to resolve this reference locally, we need a namespace scope.
if l.Namespace == "" {
return nil, fmt.Errorf("Must specify namespace to resolve reference to task %s", name)
}
return l.Tektonclient.TektonV1alpha1().Tasks(l.Namespace).Get(name, metav1.GetOptions{})
}
Loading

0 comments on commit b435bd9

Please sign in to comment.