Update build-gcs resource type to support .tar.gz archives #1200

imjasonh · 2019-08-16T04:54:41Z

Changes

support both ZipArchive and TarGzArchive types now supported by gcs-fetcher
- mark Archive type as deprecated ❌ (ZipArchive is the equivalent)
rename gcs-resource-spec-taskrun.yaml -> build-gcs-{targz,zip}.yaml to make their purposes clearer
- make the taskrun name more descriptive
- fetch a smaller file (archive.{zip,tar.gz}) containing a single file for faster tests ⏩
remove test/gcs_taskrun_test.go which didn't usefully test anything that wasn't tested in the YAML test
update docs to reflect these changes 📜

And, while I'm at it:

remove support for build-gcs upload functionality
- this doesn't appear to have worked and AFAIK nobody used it anyway
- if we want to support it, we can, but probably not like that... it only supported Manifest-style uploads, for instance 🌵
- remove gcs-uploader image from flags, and from vendor/ and from tekton/ release pipeline YAMLs 🎉

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

[ y ] Includes tests (if functionality changed/added)
[ y ] Includes docs (if user facing)
[ y ] Commit messages follow commit message best practices

See the contribution guide for more details.

Double check this list of stuff that's easy to miss:

If you are adding a new binary/image to the cmd dir, please update
the release Task and TaskRun to build and release this image

Reviewer Notes

If API changes
are included, additive changes
must be approved by at least two OWNERS
and backwards incompatible changes
must be approved by more than 50% of the OWNERS,
and they must first be added
in a backwards compatible way.

Release Notes

- build-gcs resources now support fetching and extracing .tar.gz archives

🚨 Upcoming Breaking change 🚨
- build-gcs resources will no longer support `artifactType` `Archive`, use `ZipArchive` and `TarGzArchive` instead

🚨 Breaking change 🚨
- build-gcs resources cannot be used as an output resource

tekton-robot · 2019-08-16T04:58:48Z

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1alpha1/build_gcs_resource.go	80.5%	79.4%	-1.1

tekton-robot · 2019-08-16T05:31:46Z

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1alpha1/build_gcs_resource.go	80.5%	79.4%	-1.1

tekton-robot · 2019-08-16T05:44:32Z

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1alpha1/build_gcs_resource.go	80.5%	79.4%	-1.1

imjasonh · 2019-08-16T13:05:04Z

/assign @bobcatfish

tekton-robot · 2019-08-16T14:47:32Z

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1alpha1/build_gcs_resource.go	80.5%	79.4%	-1.1

bobcatfish

Just a bunch of minor comments!

Since this does include a breaking change, we should get a couple more owners to review also - like you said its probably a fair guess that no one was using the upload functionality (cuz it wouldnt have worked anyway?) so im okay with it ( @dlorenc @afrittoli @vdemeester @dibyom @abayer and soon to be owner @sbwsg )

docs/resources.md

bobcatfish · 2019-08-16T14:34:30Z

pkg/apis/pipeline/v1alpha1/build_gcs_resource.go

 )

 // GCSArtifactType defines a type of GCS resource.
 type GCSArtifactType string

 const (
 	// GCSArchive indicates that resource should be fetched from a typical archive file.
+	//
+	// Deprecated: Use GCSZipArchive instead.
 	GCSArchive GCSArtifactType = "Archive"


can we make a follow up issue (unless you did already!) to remove this?

Do you have any concerns about me just removing it in this PR?

if we're sure no one is using it, im okay with it but we'll need a few more owners to sign off on it (2 more so we have a total of 4? 1/2 + 1)

pkg/apis/pipeline/v1alpha1/build_gcs_resource.go

pkg/apis/pipeline/v1alpha1/gcs_resource.go

tekton/publish-run.yaml

test/gcs_taskrun_test.go

tekton-robot · 2019-08-16T15:39:00Z

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1alpha1/build_gcs_resource.go	80.5%	79.4%	-1.1

tekton-robot · 2019-08-16T18:21:55Z

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1alpha1/build_gcs_resource.go	80.5%	79.4%	-1.1

- support both `ZipArchive` and `TarGzArchive` types now supported by `gcs-fetcher` - mark `Archive` type as deprecated ❌ (`ZipArchive` is the equivalent) - rename `gcs-resource-spec-taskrun.yaml` -> `build-gcs-{targz,zip}.yaml` to make their purposes clearer - make the taskrun name more descriptive - fetch a smaller file (`archive.{zip,tar.gz}`) containing a single file for faster tests ⏩ - remove `test/gcs_taskrun_test.go` which didn't usefully test anything that wasn't tested in the YAML test - update docs to reflect these changes 📜 - explicitly set `gcs-fetcher` image entrypoint And, while I'm at it: - remove support for `build-gcs` upload functionality - this doesn't appear to have worked and AFAIK nobody used it anyway - if we want to support it, we can, but probably not like that... it only supported `Manifest`-style uploads, for instance 🌵 - remove `gcs-uploader` image from flags, and from `vendor/` and from `tekton/` release pipeline YAMLs 🎉

tekton-robot · 2019-08-16T19:02:23Z

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1alpha1/build_gcs_resource.go	80.5%	79.4%	-1.1

pkg/apis/pipeline/v1alpha1/build_gcs_resource.go

dlorenc · 2019-08-16T19:11:21Z

/lgtm

bobcatfish · 2019-08-16T19:12:04Z

/hold

(gotta get one more owner)

vdemeester

/lgtm

tekton-robot · 2019-08-19T08:43:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ImJasonH, vdemeester

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [ImJasonH,vdemeester]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

imjasonh · 2019-08-19T11:12:01Z

/hold cancel

googlebot added the cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit label Aug 16, 2019

tekton-robot requested review from abayer and dlorenc August 16, 2019 04:54

tekton-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Aug 16, 2019

imjasonh force-pushed the targz branch from 35825d6 to 1d17b17 Compare August 16, 2019 05:29

imjasonh force-pushed the targz branch from 1d17b17 to 22c3584 Compare August 16, 2019 05:38

tekton-robot assigned bobcatfish Aug 16, 2019

imjasonh force-pushed the targz branch from 22c3584 to 7955ccf Compare August 16, 2019 14:45

bobcatfish reviewed Aug 16, 2019

View reviewed changes

imjasonh force-pushed the targz branch from 7955ccf to fd6f1e6 Compare August 16, 2019 15:35

imjasonh mentioned this pull request Aug 16, 2019

We aren't running bucket storage-related e2e tests in CI #821

Closed

imjasonh force-pushed the targz branch from fd6f1e6 to 5d5327e Compare August 16, 2019 18:19

imjasonh force-pushed the targz branch from 5d5327e to ae7d5c3 Compare August 16, 2019 18:58

bobcatfish reviewed Aug 16, 2019

View reviewed changes

pkg/apis/pipeline/v1alpha1/build_gcs_resource.go Show resolved Hide resolved

tekton-robot assigned dlorenc Aug 16, 2019

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 16, 2019

tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 16, 2019

vdemeester approved these changes Aug 19, 2019

View reviewed changes

tekton-robot assigned vdemeester Aug 19, 2019

tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 19, 2019

tekton-robot merged commit 5638925 into tektoncd:master Aug 19, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update build-gcs resource type to support .tar.gz archives #1200

Update build-gcs resource type to support .tar.gz archives #1200

imjasonh commented Aug 16, 2019 •

edited

Loading

tekton-robot commented Aug 16, 2019

tekton-robot commented Aug 16, 2019

tekton-robot commented Aug 16, 2019

imjasonh commented Aug 16, 2019

tekton-robot commented Aug 16, 2019

bobcatfish left a comment •

edited

Loading

bobcatfish Aug 16, 2019

imjasonh Aug 16, 2019

bobcatfish Aug 16, 2019

tekton-robot commented Aug 16, 2019

tekton-robot commented Aug 16, 2019

tekton-robot commented Aug 16, 2019

dlorenc commented Aug 16, 2019

bobcatfish commented Aug 16, 2019

vdemeester left a comment

tekton-robot commented Aug 19, 2019

imjasonh commented Aug 19, 2019

Update build-gcs resource type to support .tar.gz archives #1200

Update build-gcs resource type to support .tar.gz archives #1200

Conversation

imjasonh commented Aug 16, 2019 • edited Loading

Changes

Submitter Checklist

Reviewer Notes

Release Notes

tekton-robot commented Aug 16, 2019

tekton-robot commented Aug 16, 2019

tekton-robot commented Aug 16, 2019

imjasonh commented Aug 16, 2019

tekton-robot commented Aug 16, 2019

bobcatfish left a comment • edited Loading

Choose a reason for hiding this comment

bobcatfish Aug 16, 2019

Choose a reason for hiding this comment

imjasonh Aug 16, 2019

Choose a reason for hiding this comment

bobcatfish Aug 16, 2019

Choose a reason for hiding this comment

tekton-robot commented Aug 16, 2019

tekton-robot commented Aug 16, 2019

tekton-robot commented Aug 16, 2019

dlorenc commented Aug 16, 2019

bobcatfish commented Aug 16, 2019

vdemeester left a comment

Choose a reason for hiding this comment

tekton-robot commented Aug 19, 2019

imjasonh commented Aug 19, 2019

imjasonh commented Aug 16, 2019 •

edited

Loading

bobcatfish left a comment •

edited

Loading