Skip to content

Commit

Permalink
Address code review feedback:
Browse files Browse the repository at this point in the history
- Move logic to a new file in resources pkg.
- Switch to using a Getter from a Lister
- Add separate tests for params and resources
  • Loading branch information
dlorenc authored and knative-prow-robot committed Oct 17, 2018
1 parent 170e628 commit 3b03577
Show file tree
Hide file tree
Showing 5 changed files with 308 additions and 37 deletions.
37 changes: 37 additions & 0 deletions pkg/apis/pipeline/v1alpha1/image_resource.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,33 @@ limitations under the License.

package v1alpha1

import (
"fmt"
"strings"
)

// NewImageResource creates a new ImageResource from a PipelineResource.
func NewImageResource(r *PipelineResource) (*ImageResource, error) {
if r.Spec.Type != PipelineResourceTypeImage {
return nil, fmt.Errorf("ImageResource: Cannot create an Image resource from a %s Pipeline Resource", r.Spec.Type)
}
ir := &ImageResource{
Name: r.Name,
Type: PipelineResourceTypeImage,
}

for _, param := range r.Spec.Params {
switch {
case strings.EqualFold(param.Name, "URL"):
ir.URL = param.Value
case strings.EqualFold(param.Name, "Digest"):
ir.Digest = param.Value
}
}

return ir, nil
}

// ImageResource defines an endpoint where artifacts can be stored, such as images.
type ImageResource struct {
Name string `json:"name"`
Expand All @@ -41,3 +68,13 @@ func (s ImageResource) GetVersion() string {

// GetParams returns the resoruce params
func (s ImageResource) GetParams() []Param { return []Param{} }

// Replacements is used for template replacement on an ImageResource inside of a Taskrun.
func (s *ImageResource) Replacements() map[string]string {
return map[string]string{
"name": s.Name,
"type": string(s.Type),
"url": s.URL,
"digest": s.Digest,
}
}
6 changes: 4 additions & 2 deletions pkg/apis/pipeline/v1alpha1/resource_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -116,11 +116,13 @@ type PipelineResourceList struct {
Items []PipelineResource `json:"items"`
}

// ResourceFromType returns a
// ResourceFromType returns a PipelineResourceInterface from a PipelineResource's type.
func ResourceFromType(r *PipelineResource) (PipelineResourceInterface, error) {
switch r.Spec.Type {
case PipelineResourceTypeGit:
return NewGitResource(r)
case PipelineResourceTypeImage:
return NewImageResource(r)
}
return nil, fmt.Errorf("%s is an invalid PipelineResource", r.Spec.Type)
return nil, fmt.Errorf("%s is an invalid or unimplemented PipelineResource", r.Spec.Type)
}
61 changes: 61 additions & 0 deletions pkg/reconciler/v1alpha1/taskrun/resources/apply.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
/*
Copyright 2018 The Knative 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/knative/build-pipeline/pkg/apis/pipeline/v1alpha1"
buildv1alpha1 "github.com/knative/build/pkg/apis/build/v1alpha1"
"github.com/knative/build/pkg/builder"
)

// ApplyParameters applies the params from a TaskRun.Input.Parameters to a BuildSpec.
func ApplyParameters(b *buildv1alpha1.Build, tr *v1alpha1.TaskRun) *buildv1alpha1.Build {
// This assumes that the TaskRun inputs have been validated against what the Task requests.
replacements := map[string]string{}
for _, p := range tr.Spec.Inputs.Params {
replacements[fmt.Sprintf("inputs.params.%s", p.Name)] = p.Value
}

return builder.ApplyReplacements(b, replacements)
}

type ResourceGetter interface {
Get(string) (*v1alpha1.PipelineResource, error)
}

// ApplyResources applies the params from a TaskRun.Input.Resources to a BuildSpec.
func ApplyResources(b *buildv1alpha1.Build, tr *v1alpha1.TaskRun, getter ResourceGetter) (*buildv1alpha1.Build, error) {
replacements := map[string]string{}

for _, ir := range tr.Spec.Inputs.Resources {
pr, err := getter.Get(ir.ResourceRef.Name)
if err != nil {
return nil, err
}

resource, err := v1alpha1.ResourceFromType(pr)
if err != nil {
return nil, err
}
for k, v := range resource.Replacements() {
replacements[fmt.Sprintf("inputs.resources.%s.%s", ir.ResourceRef.Name, k)] = v
}
}
return builder.ApplyReplacements(b, replacements), nil
}
201 changes: 201 additions & 0 deletions pkg/reconciler/v1alpha1/taskrun/resources/apply_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,201 @@
/*
Copyright 2018 The Knative 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"
"testing"

"github.com/google/go-cmp/cmp"
"github.com/knative/build-pipeline/pkg/apis/pipeline/v1alpha1"
buildv1alpha1 "github.com/knative/build/pkg/apis/build/v1alpha1"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
)

var simpleBuild = &buildv1alpha1.Build{
Spec: buildv1alpha1.BuildSpec{
Steps: []corev1.Container{
{
Name: "foo",
Image: "${inputs.params.myimage}",
},
{
Name: "baz",
Image: "bat",
Args: []string{"${inputs.resources.git-resource.url}"},
},
},
},
}

var paramTaskRun = &v1alpha1.TaskRun{
Spec: v1alpha1.TaskRunSpec{
Inputs: v1alpha1.TaskRunInputs{
Params: []v1alpha1.Param{
{
Name: "myimage",
Value: "bar",
},
},
},
},
}

var resourceTaskRun = &v1alpha1.TaskRun{
Spec: v1alpha1.TaskRunSpec{
Inputs: v1alpha1.TaskRunInputs{
Resources: []v1alpha1.PipelineResourceVersion{
{
ResourceRef: v1alpha1.PipelineResourceRef{
Name: "git-resource",
},
},
},
},
},
}

var gitResource = &v1alpha1.PipelineResource{
ObjectMeta: metav1.ObjectMeta{
Name: "git-resource",
},
Spec: v1alpha1.PipelineResourceSpec{
Type: v1alpha1.PipelineResourceTypeGit,
Params: []v1alpha1.Param{
{
Name: "URL",
Value: "https://git-repo",
},
},
},
}

func applyMutation(b *buildv1alpha1.Build, f func(b *buildv1alpha1.Build)) *buildv1alpha1.Build {
b = b.DeepCopy()
f(b)
return b
}

func TestApplyParameters(t *testing.T) {
type args struct {
b *buildv1alpha1.Build
tr *v1alpha1.TaskRun
}
tests := []struct {
name string
args args
want *buildv1alpha1.Build
}{
{
name: "single parameter",
args: args{
b: simpleBuild,
tr: paramTaskRun,
},
want: applyMutation(simpleBuild, func(b *buildv1alpha1.Build) {
b.Spec.Steps[0].Image = "bar"
}),
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
got := ApplyParameters(tt.args.b, tt.args.tr)
if d := cmp.Diff(got, tt.want); d != "" {
t.Errorf("ApplyParameters() got diff %s", d)
}
})
}
}

type rg struct {
resources map[string]*v1alpha1.PipelineResource
}

func (rg *rg) Get(name string) (*v1alpha1.PipelineResource, error) {
if pr, ok := rg.resources[name]; ok {
return pr, nil
}
return nil, fmt.Errorf("Resource %s does not exist.", name)
}

func (rg *rg) With(name string, pr *v1alpha1.PipelineResource) *rg {
rg.resources[name] = pr
return rg
}

func mockGetter() *rg {
return &rg{
resources: map[string]*v1alpha1.PipelineResource{},
}
}

func TestApplyResources(t *testing.T) {
type args struct {
b *buildv1alpha1.Build
tr *v1alpha1.TaskRun
getter ResourceGetter
}
tests := []struct {
name string
args args
want *buildv1alpha1.Build
wantErr bool
}{
{
name: "no replacements specified",
args: args{
b: simpleBuild,
tr: paramTaskRun,
getter: mockGetter(),
},
want: simpleBuild,
},
{
name: "resource specified",
args: args{
b: simpleBuild,
tr: resourceTaskRun,
getter: mockGetter().With("git-resource", gitResource),
},
want: applyMutation(simpleBuild, func(b *buildv1alpha1.Build) {
b.Spec.Steps[1].Args = []string{"https://git-repo"}
}),
},
{
name: "resource does not exist",
args: args{
b: simpleBuild,
tr: resourceTaskRun,
getter: mockGetter(),
},
wantErr: true,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
got, err := ApplyResources(tt.args.b, tt.args.tr, tt.args.getter)
if (err != nil) != tt.wantErr {
t.Errorf("ApplyResources() error = %v, wantErr %v", err, tt.wantErr)
return
}
if d := cmp.Diff(got, tt.want); d != "" {
t.Errorf("ApplyResources() diff %s", d)
}
})
}
}
40 changes: 5 additions & 35 deletions pkg/reconciler/v1alpha1/taskrun/taskrun.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,10 @@ import (
"fmt"
"reflect"

"github.com/knative/build-pipeline/pkg/apis/pipeline/v1alpha1"
"github.com/knative/build-pipeline/pkg/reconciler"
resources "github.com/knative/build-pipeline/pkg/reconciler/v1alpha1/taskrun/resources"
buildv1alpha1 "github.com/knative/build/pkg/apis/build/v1alpha1"
"github.com/knative/build/pkg/builder"
buildinformers "github.com/knative/build/pkg/client/informers/externalversions/build/v1alpha1"
buildlisters "github.com/knative/build/pkg/client/listers/build/v1alpha1"
duckv1alpha1 "github.com/knative/pkg/apis/duck/v1alpha1"
Expand All @@ -35,11 +37,8 @@ import (
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/client-go/tools/cache"

"github.com/knative/build-pipeline/pkg/apis/pipeline/v1alpha1"
informers "github.com/knative/build-pipeline/pkg/client/informers/externalversions/pipeline/v1alpha1"
listers "github.com/knative/build-pipeline/pkg/client/listers/pipeline/v1alpha1"
"github.com/knative/build-pipeline/pkg/reconciler"
resources "github.com/knative/build-pipeline/pkg/reconciler/v1alpha1/taskrun/resources"
)

const (
Expand Down Expand Up @@ -233,10 +232,10 @@ func (c *Reconciler) createBuild(tr *v1alpha1.TaskRun) (*buildv1alpha1.Build, er
}

// Apply parameters from the taskrun.
build = applyParameters(build, t, tr)
build = resources.ApplyParameters(build, tr)

// Apply resources from the taskrun.
build, err = applyResources(build, t, tr, c.resourceLister)
build, err = resources.ApplyResources(build, tr, c.resourceLister.PipelineResources(t.Namespace))
if err != nil {
return nil, err
}
Expand All @@ -254,32 +253,3 @@ func makeLabels(s *v1alpha1.TaskRun) map[string]string {
return labels

}
func applyParameters(b *buildv1alpha1.Build, t *v1alpha1.Task, tr *v1alpha1.TaskRun) *buildv1alpha1.Build {
// This assumes that the TaskRun inputs have been validated against what the Task requests.
replacements := map[string]string{}
for _, p := range tr.Spec.Inputs.Params {
replacements[fmt.Sprintf("inputs.params.%s", p.Name)] = p.Value
}

return builder.ApplyReplacements(b, replacements)
}

func applyResources(b *buildv1alpha1.Build, t *v1alpha1.Task, tr *v1alpha1.TaskRun, lister listers.PipelineResourceLister) (*buildv1alpha1.Build, error) {
replacements := map[string]string{}

for _, ir := range tr.Spec.Inputs.Resources {
pr, err := lister.PipelineResources(t.Namespace).Get(ir.ResourceRef.Name)
if err != nil {
return nil, err
}

resource, err := v1alpha1.ResourceFromType(pr)
if err != nil {
return nil, err
}
for k, v := range resource.Replacements() {
replacements[fmt.Sprintf("inputs.resources.%s.%s", ir.ResourceRef.Name, k)] = v
}
}
return builder.ApplyReplacements(b, replacements), nil
}

0 comments on commit 3b03577

Please sign in to comment.