Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Only copy output resource to PVC for storage/git output #414

Merged
merged 2 commits into from
Jan 22, 2019
Merged

Only copy output resource to PVC for storage/git output #414

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8ddbc96
Onpy copy output resource to PVC for storage/git output
afrittoli Jan 21, 2019
b116f07
Move the allowed output resource to a map
afrittoli Jan 21, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 19 additions & 1 deletion pkg/reconciler/v1alpha1/taskrun/resources/output_resource.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,13 @@ import (

var (
outputDir = "/workspace/output/"

// allowedOutputResource checks if an output resource type produces
// an output that should be copied to the PVC
allowedOutputResources = map[v1alpha1.PipelineResourceType]bool{
v1alpha1.PipelineResourceTypeStorage: true,
v1alpha1.PipelineResourceTypeGit: true,
}
)

// AddOutputResources reads the output resources and adds the corresponding container steps
Expand Down Expand Up @@ -105,8 +112,12 @@ func AddOutputResources(
}
}

// Workaround for issue #401. Unless all resource types are implemented as
// outputs, or until we have metadata on the resource that declares whether
// the output should be copied to the PVC, only copy git and storage output
// resources.
// copy to pvc if pvc is present
if pipelineRunpvcName != "" {
if allowedOutputResources[resource.Spec.Type] && pipelineRunpvcName != "" {
var newSteps []corev1.Container
for _, dPath := range boundResource.Paths {
newSteps = append(newSteps, []corev1.Container{{
Expand Down Expand Up @@ -186,3 +197,10 @@ func addStoreUploadStep(build *buildv1alpha1.Build,
build.Spec.Steps = append(build.Spec.Steps, buildSteps...)
return nil
}

// allowedOutputResource checks if an output resource type produces
// an output that should be copied to the PVC
func allowedOutputResource(resourceType v1alpha1.PipelineResourceType) bool {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

allowedOutputResources is not going to change so makes sense to define it in variable section. Something like

var (
// allowedOutputResource checks if an output resource type produces
// an output that should be copied to the PVC
	allowedOutputResources = map[v1alpha1.PipelineResourceType]bool{
		v1alpha1.PipelineResourceTypeStorage: true,
		v1alpha1.PipelineResourceTypeGit:     true,
	}
)

and update the if condition to be

if allowedOutputResources[resource.Spec.Type] && pipelineRunpvcName != "" {
}

Code is simple and does not need a function. WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made a function because you asked for it to begin with :)

I'm not sure that list is not going to change... other resources will have output to be copied, like logs of the build process for the image resource.
I hope eventually that list will go away for good and it will be replaced by a proper interface on the resource to declare whether they have output to be copied or not.

The advantage of having a function is also that it's possible to write a simple unit test for it, but it's true that there's little value in that unit test, so I'm ok with dropping the function if you prefer so.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made a function because you asked for it to begin with :)

Sorry for the confusion. My intention was to make the if condition simpler and easier to read so I jumped at function as solution. I should have left up to you come up with simpler solution instead of influencing my solution on you. After I saw your solution it felt declaring map in variables section seemed a lot simpler than function

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The advantage of having a function is also that it's possible to write a simple unit test for it, but it's true that there's little value in that unit test, so I'm ok with dropping the function if you prefer so.

You can still have the tests. I think it holds value in showing supported output types explicitly. I like the tests. I have suggested below how to update tests to use map instead of function.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, addressed everything - hopefully - in the version :)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hope eventually that list will go away for good and it will be replaced by a proper interface on the resource to declare whether they have output to be copied or not.

yuusssss


return allowedOutputResources[resourceType]
}
73 changes: 73 additions & 0 deletions pkg/reconciler/v1alpha1/taskrun/resources/output_resource_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,14 @@ func outputResourcesetUp() {
FieldName: "STORAGE_CREDS",
}},
},
}, {
ObjectMeta: metav1.ObjectMeta{
Name: "source-image",
Namespace: "marshmallow",
},
Spec: v1alpha1.PipelineResourceSpec{
Type: "image",
},
}}

for _, r := range rs {
Expand Down Expand Up @@ -549,6 +557,45 @@ func Test_Valid_OutputResources(t *testing.T) {
Secret: &corev1.SecretVolumeSource{SecretName: "sname"},
},
}},
}, {
name: "image resource as output",
desc: "image resource defined only in output",
taskRun: &v1alpha1.TaskRun{
ObjectMeta: metav1.ObjectMeta{
Name: "test-taskrun-run-only-output-step",
Namespace: "marshmallow",
OwnerReferences: []metav1.OwnerReference{{
Kind: "PipelineRun",
Name: "pipelinerun",
}},
},
Spec: v1alpha1.TaskRunSpec{
Outputs: v1alpha1.TaskRunOutputs{
Resources: []v1alpha1.TaskResourceBinding{{
Name: "source-workspace",
ResourceRef: v1alpha1.PipelineResourceRef{
Name: "source-image",
},
}},
},
},
},
task: &v1alpha1.Task{
ObjectMeta: metav1.ObjectMeta{
Name: "task1",
Namespace: "marshmallow",
},
Spec: v1alpha1.TaskSpec{
Outputs: &v1alpha1.Outputs{
Resources: []v1alpha1.TaskResource{{
Name: "source-workspace",
Type: "image",
}},
},
},
},
wantSteps: nil,
build: build(),
}} {
t.Run(c.name, func(t *testing.T) {
outputResourcesetUp()
Expand Down Expand Up @@ -716,3 +763,29 @@ func Test_InValid_OutputResources(t *testing.T) {
})
}
}

func Test_AllowedOutputResource(t *testing.T) {
for _, c := range []struct {
desc string
resourceType v1alpha1.PipelineResourceType
expectedAllowed bool
}{{
desc: "storage type is allowed",
resourceType: v1alpha1.PipelineResourceTypeStorage,
expectedAllowed: true,
}, {
desc: "git type is allowed",
resourceType: v1alpha1.PipelineResourceTypeGit,
expectedAllowed: true,
}, {
desc: "anything else is not allowed",
resourceType: "fooType",
expectedAllowed: false,
}} {
t.Run(c.desc, func(t *testing.T) {
if c.expectedAllowed != allowedOutputResources[c.resourceType] {
t.Fatalf("Test AllowedOutputResource %s expected %t but got %t", c.desc, c.expectedAllowed, allowedOutputResources[c.resourceType])
}
})
}
}