Skip to content

Commit d905b3e

Browse files
dlorenctekton-robot
dlorenc
authored and
tekton-robot
committed
Refactor Resource interface a bit to simplify directory passing.
This change removes the SetDestinationDirectory method from the Resource interface. This overcomplicated each Resource, and is unnecessary. The directory resources need to copy/paste from can be simply passed in at container generation time.
1 parent 8272358 commit d905b3e

18 files changed

+137
-182
lines changed

Diff for: ‎pkg/apis/pipeline/v1alpha1/build_gcs_resource.go

+11-19
Original file line numberDiff line numberDiff line change
@@ -53,11 +53,10 @@ const (
5353
// BuildGCSResource does incremental uploads for files in directory.
5454

5555
type BuildGCSResource struct {
56-
Name string
57-
Type PipelineResourceType
58-
Location string
59-
DestinationDir string
60-
ArtifactType GCSArtifactType
56+
Name string
57+
Type PipelineResourceType
58+
Location string
59+
ArtifactType GCSArtifactType
6160
}
6261

6362
// NewBuildGCSResource creates a new BuildGCS resource to pass to a Task
@@ -116,26 +115,19 @@ func (s *BuildGCSResource) Replacements() map[string]string {
116115
"name": s.Name,
117116
"type": string(s.Type),
118117
"location": s.Location,
119-
"path": s.DestinationDir,
120118
}
121119
}
122120

123-
// SetDestinationDirectory sets the destination directory at runtime like where is the resource going to be copied to
124-
func (s *BuildGCSResource) SetDestinationDirectory(destDir string) { s.DestinationDir = destDir }
125-
126121
// GetDownloadContainerSpec returns an array of container specs to download gcs storage object
127-
func (s *BuildGCSResource) GetDownloadContainerSpec() ([]corev1.Container, error) {
128-
if s.DestinationDir == "" {
129-
return nil, xerrors.Errorf("BuildGCSResource: Expect Destination Directory param to be set %s", s.Name)
130-
}
122+
func (s *BuildGCSResource) GetDownloadContainerSpec(sourcePath string) ([]corev1.Container, error) {
131123
args := []string{"--type", string(s.ArtifactType), "--location", s.Location}
132124
// dest_dir is the destination directory for GCS files to be copies"
133-
if s.DestinationDir != "" {
134-
args = append(args, "--dest_dir", s.DestinationDir)
125+
if sourcePath != "" {
126+
args = append(args, "--dest_dir", sourcePath)
135127
}
136128

137129
return []corev1.Container{
138-
CreateDirContainer(s.Name, s.DestinationDir), {
130+
CreateDirContainer(s.Name, sourcePath), {
139131
Name: names.SimpleNameGenerator.RestrictLengthWithRandomSuffix(fmt.Sprintf("storage-fetch-%s", s.Name)),
140132
Image: *buildGCSFetcherImage,
141133
Args: args,
@@ -144,14 +136,14 @@ func (s *BuildGCSResource) GetDownloadContainerSpec() ([]corev1.Container, error
144136

145137
// GetUploadContainerSpec gets container spec for gcs resource to be uploaded like
146138
// set environment variable from secret params and set volume mounts for those secrets
147-
func (s *BuildGCSResource) GetUploadContainerSpec() ([]corev1.Container, error) {
139+
func (s *BuildGCSResource) GetUploadContainerSpec(sourcePath string) ([]corev1.Container, error) {
148140
if s.ArtifactType != GCSManifest {
149141
return nil, xerrors.Errorf("BuildGCSResource: Can only upload Artifacts of type Manifest: %s", s.Name)
150142
}
151-
if s.DestinationDir == "" {
143+
if sourcePath == "" {
152144
return nil, xerrors.Errorf("BuildGCSResource: Expect Destination Directory param to be set %s", s.Name)
153145
}
154-
args := []string{"--location", s.Location, "--dir", s.DestinationDir}
146+
args := []string{"--location", s.Location, "--dir", sourcePath}
155147

156148
return []corev1.Container{{
157149
Name: names.SimpleNameGenerator.RestrictLengthWithRandomSuffix(fmt.Sprintf("storage-upload-%s", s.Name)),

Diff for: ‎pkg/apis/pipeline/v1alpha1/build_gcs_resource_test.go

+12-31
Original file line numberDiff line numberDiff line change
@@ -121,7 +121,6 @@ func Test_BuildGCSGetReplacements(t *testing.T) {
121121
"name": "gcs-resource",
122122
"type": "build-gcs",
123123
"location": "gs://fake-bucket",
124-
"path": "",
125124
}
126125
if d := cmp.Diff(r.Replacements(), expectedReplacementMap); d != "" {
127126
t.Errorf("BuildGCS Replacement map mismatch: %s", d)
@@ -137,10 +136,9 @@ func Test_BuildGCSGetDownloadContainerSpec(t *testing.T) {
137136
}{{
138137
name: "valid download protected buckets",
139138
resource: &v1alpha1.BuildGCSResource{
140-
Name: "gcs-valid",
141-
Location: "gs://some-bucket",
142-
DestinationDir: "/workspace",
143-
ArtifactType: "Archive",
139+
Name: "gcs-valid",
140+
Location: "gs://some-bucket",
141+
ArtifactType: "Archive",
144142
},
145143
wantContainers: []corev1.Container{{
146144
Name: "create-dir-gcs-valid-9l9zj",
@@ -153,19 +151,11 @@ func Test_BuildGCSGetDownloadContainerSpec(t *testing.T) {
153151
Args: []string{"--type", "Archive", "--location", "gs://some-bucket",
154152
"--dest_dir", "/workspace"},
155153
}},
156-
}, {
157-
name: "invalid no destination directory set",
158-
resource: &v1alpha1.BuildGCSResource{
159-
Name: "gcs-invalid",
160-
Location: "gs://some-bucket",
161-
ArtifactType: "Archive",
162-
},
163-
wantErr: true,
164154
}}
165155
for _, tc := range testcases {
166156
t.Run(tc.name, func(t *testing.T) {
167157
names.TestingSeed()
168-
gotContainers, err := tc.resource.GetDownloadContainerSpec()
158+
gotContainers, err := tc.resource.GetDownloadContainerSpec("/workspace")
169159
if tc.wantErr && err == nil {
170160
t.Fatalf("Expected error to be %t but got %v:", tc.wantErr, err)
171161
}
@@ -185,36 +175,27 @@ func Test_BuildGCSGetUploadContainerSpec(t *testing.T) {
185175
}{{
186176
name: "valid upload to protected buckets with directory paths",
187177
resource: &v1alpha1.BuildGCSResource{
188-
Name: "gcs-valid",
189-
Location: "gs://some-bucket/manifest.json",
190-
DestinationDir: "/workspace",
191-
ArtifactType: "Manifest",
178+
Name: "gcs-valid",
179+
Location: "gs://some-bucket/manifest.json",
180+
ArtifactType: "Manifest",
192181
},
193182
wantContainers: []corev1.Container{{
194-
Name: "storage-upload-gcs-valid-9l9zj",
183+
Name: "storage-upload-gcs-valid-mssqb",
195184
Image: "gcr.io/cloud-builders/gcs-uploader:latest",
196185
Args: []string{"--location", "gs://some-bucket/manifest.json", "--dir", "/workspace"},
197186
}},
198187
}, {
199188
name: "invalid upload to protected buckets with single file",
200189
resource: &v1alpha1.BuildGCSResource{
201-
Name: "gcs-valid",
202-
ArtifactType: "Archive",
203-
Location: "gs://some-bucket",
204-
DestinationDir: "/workspace/results.tar",
205-
},
206-
wantErr: true,
207-
}, {
208-
name: "invalid upload with no source directory path",
209-
resource: &v1alpha1.BuildGCSResource{
210-
Name: "gcs-invalid",
211-
Location: "gs://some-bucket/manifest.json",
190+
Name: "gcs-valid",
191+
ArtifactType: "Archive",
192+
Location: "gs://some-bucket",
212193
},
213194
wantErr: true,
214195
}}
215196
for _, tc := range testcases {
216197
t.Run(tc.name, func(t *testing.T) {
217-
gotContainers, err := tc.resource.GetUploadContainerSpec()
198+
gotContainers, err := tc.resource.GetUploadContainerSpec("/workspace")
218199
if tc.wantErr && err == nil {
219200
t.Fatalf("Expected error to be %t but got %v:", tc.wantErr, err)
220201
}

Diff for: ‎pkg/apis/pipeline/v1alpha1/cloud_event_resource.go

+2-6
Original file line numberDiff line numberDiff line change
@@ -81,19 +81,15 @@ func (s *CloudEventResource) Replacements() map[string]string {
8181
}
8282

8383
// GetUploadContainerSpec returns nothing as the cloud event is sent by the controller once the POD execution is completed
84-
func (s *CloudEventResource) GetUploadContainerSpec() ([]corev1.Container, error) {
84+
func (s *CloudEventResource) GetUploadContainerSpec(_ string) ([]corev1.Container, error) {
8585
return nil, nil
8686
}
8787

8888
// GetDownloadContainerSpec returns nothing, cloud events cannot be used as input resource
89-
func (s *CloudEventResource) GetDownloadContainerSpec() ([]corev1.Container, error) {
89+
func (s *CloudEventResource) GetDownloadContainerSpec(_ string) ([]corev1.Container, error) {
9090
return nil, nil
9191
}
9292

93-
// SetDestinationDirectory required by the interface but not really used
94-
func (s *CloudEventResource) SetDestinationDirectory(path string) {
95-
}
96-
9793
// GetUploadVolumeSpec - no upload from volume for CloudEvent resource
9894
func (s *CloudEventResource) GetUploadVolumeSpec(spec *TaskSpec) ([]corev1.Volume, error) {
9995
return nil, nil

Diff for: ‎pkg/apis/pipeline/v1alpha1/cloud_event_resource_test.go

+2-2
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,7 @@ func Test_CloudEventDownloadContainerSpec(t *testing.T) {
9494
TargetURI: "http://fake-uri",
9595
Type: v1alpha1.PipelineResourceTypeCloudEvent,
9696
}
97-
d, e := r.GetDownloadContainerSpec()
97+
d, e := r.GetDownloadContainerSpec("")
9898
if d != nil {
9999
t.Errorf("Did not expect a download container for CloudEventResource")
100100
}
@@ -109,7 +109,7 @@ func Test_CloudEventUploadContainerSpec(t *testing.T) {
109109
TargetURI: "http://fake-uri",
110110
Type: v1alpha1.PipelineResourceTypeCloudEvent,
111111
}
112-
d, e := r.GetUploadContainerSpec()
112+
d, e := r.GetUploadContainerSpec("")
113113
if d != nil {
114114
t.Errorf("Did not expect an upload container for CloudEventResource")
115115
}

Diff for: ‎pkg/apis/pipeline/v1alpha1/cluster_resource.go

+2-4
Original file line numberDiff line numberDiff line change
@@ -138,13 +138,11 @@ func (s ClusterResource) String() string {
138138
return string(json)
139139
}
140140

141-
func (s *ClusterResource) GetUploadContainerSpec() ([]corev1.Container, error) {
141+
func (s *ClusterResource) GetUploadContainerSpec(_ string) ([]corev1.Container, error) {
142142
return nil, nil
143143
}
144144

145-
func (s *ClusterResource) SetDestinationDirectory(path string) {
146-
}
147-
func (s *ClusterResource) GetDownloadContainerSpec() ([]corev1.Container, error) {
145+
func (s *ClusterResource) GetDownloadContainerSpec(sourcePath string) ([]corev1.Container, error) {
148146
var envVars []corev1.EnvVar
149147
for _, sec := range s.Secrets {
150148
ev := corev1.EnvVar{

Diff for: ‎pkg/apis/pipeline/v1alpha1/cluster_resource_test.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -156,7 +156,7 @@ func Test_ClusterResource_GetDownloadContainerSpec(t *testing.T) {
156156
}}
157157
for _, tc := range testcases {
158158
t.Run(tc.name, func(t *testing.T) {
159-
gotContainers, err := tc.clusterResource.GetDownloadContainerSpec()
159+
gotContainers, err := tc.clusterResource.GetDownloadContainerSpec("")
160160
if tc.wantErr && err == nil {
161161
t.Fatalf("Expected error to be %t but got %v:", tc.wantErr, err)
162162
}

Diff for: ‎pkg/apis/pipeline/v1alpha1/gcs_resource.go

+13-20
Original file line numberDiff line numberDiff line change
@@ -35,11 +35,10 @@ var (
3535
// GCSResource is a GCS endpoint from which to get artifacts which is required
3636
// by a Build/Task for context (e.g. a archive from which to build an image).
3737
type GCSResource struct {
38-
Name string `json:"name"`
39-
Type PipelineResourceType `json:"type"`
40-
Location string `json:"location"`
41-
TypeDir bool `json:"typeDir"`
42-
DestinationDir string `json:"destinationDir"`
38+
Name string `json:"name"`
39+
Type PipelineResourceType `json:"type"`
40+
Location string `json:"location"`
41+
TypeDir bool `json:"typeDir"`
4342
//Secret holds a struct to indicate a field name and corresponding secret name to populate it
4443
Secrets []SecretParam `json:"secrets"`
4544
}
@@ -95,24 +94,18 @@ func (s *GCSResource) Replacements() map[string]string {
9594
"name": s.Name,
9695
"type": string(s.Type),
9796
"location": s.Location,
98-
"path": s.DestinationDir,
9997
}
10098
}
10199

102-
// SetDestinationDirectory sets the destination directory at runtime like where is the resource going to be copied to
103-
func (s *GCSResource) SetDestinationDirectory(destDir string) { s.DestinationDir = destDir }
104-
105100
// GetUploadContainerSpec gets container spec for gcs resource to be uploaded like
106101
// set environment variable from secret params and set volume mounts for those secrets
107-
func (s *GCSResource) GetUploadContainerSpec() ([]corev1.Container, error) {
108-
if s.DestinationDir == "" {
109-
return nil, xerrors.Errorf("GCSResource: Expect Destination Directory param to be set: %s", s.Name)
110-
}
102+
func (s *GCSResource) GetUploadContainerSpec(sourcePath string) ([]corev1.Container, error) {
103+
111104
var args []string
112105
if s.TypeDir {
113-
args = []string{"-args", fmt.Sprintf("rsync -d -r %s %s", s.DestinationDir, s.Location)}
106+
args = []string{"-args", fmt.Sprintf("rsync -d -r %s %s", sourcePath, s.Location)}
114107
} else {
115-
args = []string{"-args", fmt.Sprintf("cp %s %s", filepath.Join(s.DestinationDir, "*"), s.Location)}
108+
args = []string{"-args", fmt.Sprintf("cp %s %s", filepath.Join(sourcePath, "*"), s.Location)}
116109
}
117110

118111
envVars, secretVolumeMount := getSecretEnvVarsAndVolumeMounts(s.Name, gcsSecretVolumeMountPath, s.Secrets)
@@ -128,20 +121,20 @@ func (s *GCSResource) GetUploadContainerSpec() ([]corev1.Container, error) {
128121
}
129122

130123
// GetDownloadContainerSpec returns an array of container specs to download gcs storage object
131-
func (s *GCSResource) GetDownloadContainerSpec() ([]corev1.Container, error) {
132-
if s.DestinationDir == "" {
124+
func (s *GCSResource) GetDownloadContainerSpec(sourcePath string) ([]corev1.Container, error) {
125+
if sourcePath == "" {
133126
return nil, xerrors.Errorf("GCSResource: Expect Destination Directory param to be set %s", s.Name)
134127
}
135128
var args []string
136129
if s.TypeDir {
137-
args = []string{"-args", fmt.Sprintf("rsync -d -r %s %s", s.Location, s.DestinationDir)}
130+
args = []string{"-args", fmt.Sprintf("rsync -d -r %s %s", s.Location, sourcePath)}
138131
} else {
139-
args = []string{"-args", fmt.Sprintf("cp %s %s", s.Location, s.DestinationDir)}
132+
args = []string{"-args", fmt.Sprintf("cp %s %s", s.Location, sourcePath)}
140133
}
141134

142135
envVars, secretVolumeMount := getSecretEnvVarsAndVolumeMounts(s.Name, gcsSecretVolumeMountPath, s.Secrets)
143136
return []corev1.Container{
144-
CreateDirContainer(s.Name, s.DestinationDir), {
137+
CreateDirContainer(s.Name, sourcePath), {
145138
Name: names.SimpleNameGenerator.RestrictLengthWithRandomSuffix(fmt.Sprintf("fetch-%s", s.Name)),
146139
Image: *gsutilImage,
147140
Command: []string{"/ko-app/gsutil"},

0 commit comments

Comments
 (0)