Oci tool: makes use of oci-artifacts to store and retrieve Tekton resources #461

sthaha · 2020-02-12T07:46:04Z

Changes

This PR relates to the #1839: Versioning of Tasks discussion. The PR adds an oci tool that is currently able to store and retrieve Tekton resource (Pipeline, Task, ClusterTask...) from a registry.

Credits: Thanks for Pierre Tasci @pierretasci for the initial implementation.

Submitter Checklist

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

[🙅‍♂ ] Includes docs (if user facing)
Commit messages follow commit message best practices

See the contribution guide
for more details.

googlebot · 2020-02-12T07:46:15Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

This contains a POC of a cli that can push and pull tekton resources as OCI artifacts from a supported registry. Credits: Thanks to Pierre Tasci for this patch. Signed-off-by: Sunil Thaha <sthaha@redhat.com>

googlebot · 2020-02-12T12:25:57Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

imjasonh · 2020-02-17T02:23:49Z

oci/pkg/oci/image.go

+}
+
+// getNameParts will attempt to return a { "hostname/image-name", "tag or sha" } list of the provided image name. There maybe less than the full 2 elements if one of the parts isn't included in the original name.
+func getNameParts(name string) (*ImageReference, error) {


The go-containerregistry library has a pretty solid set of methods for this kind of name parsing, I'd love to avoid having to maintain it in two places.

https://godoc.org/github.com/google/go-containerregistry/pkg/name#ParseReference

imjasonh · 2020-02-17T02:24:57Z

oci/pkg/oci/bundle.go

+}
+
+// ReadPaths will recursively search each file path for Tekton resources and return the parsed specs or an error.
+func ReadPaths(filePaths []string) ([]ParsedTektonResource, error) {


It looks like a lot of these methods are only called from within this package, so they don't have to be exported.

I believe ReadPaths and ParsedTektonResource are the only exported members and they are referenced outside of the oci package inside of the action package. Perhaps there is a case to be made for re-organizing but this tends to follow a cobra cli layout as far as I can tell.

imjasonh · 2020-02-17T02:26:07Z

oci/pkg/oci/bundle.go

+	// entities in a single file).
+	var entities []string
+	switch ext := path.Ext(filePath); true {
+	case ext == ".yaml" || ext == ".yml":


I'm pretty sure there's a standard K8s package to universally decode K8s config files. Can we use that instead?

This is done in tekdoc: https://github.com/tektoncd/experimental/blob/master/tekdoc/main.go#L45

The UniversalDeserializer is being used here: https://github.com/tektoncd/experimental/pull/461/files/28fc8d52254c72a62a82d422db91ff4ecaf11b0a#diff-be720fcc306c3d06da4d0c6eac9ed71eR126 to decode the file contents and ensure they are parseable as Tekton crds.

The problem is that the deserializer, AFAIK, will only parse a single resource from a set of contents even if the file is a --- separated yaml config or a json array. We want to allow parsing multiple resources from a single file. If we take a look at how kubectl accomplishes this in the cli-runtime it is a bit more roundabout than what is trying to be accomplished here: https://github.com/kubernetes/kubectl/blob/4e4ec7cb5beac1fe75fd4be05d77c65030c30088/pkg/cmd/apply/apply.go#L323

To that end, for the first iteration of a tool that is meant just to be a POC, it might be worthwhile to keep the logic simple and more bespoke while we figure out how Tekton will support bundling these images. Perhaps we decide only Catalog CRDs should be bundled and we don't need parsing input at all"

imjasonh · 2020-02-17T02:27:35Z

oci/pkg/oci/image.go

+)
+
+func init() {
+	orascontext.GetLogger(context.Background()).Logger.SetLevel(logrus.ErrorLevel)


If we expect/hope this code will end up in either tkn or Tekton cobtrollers we should try to standardize on the logging used in those codebases, I don't think they use logrus today.

I believe this is actually just here to suppress the built-in logger of the oras package which spits out warnings for using a custom mime that isn't publicly registered anywhere. I don't think this PR specifies a logger and is, unfortunately, only importing logrus to set the log level.

imjasonh · 2020-02-17T02:28:55Z

oci/pkg/oci/image.go

+	resolver := docker.NewResolver(docker.ResolverOptions{})
+	memoryStore := orascontent.NewMemoryStore()
+
+	_, _, err := oras.Pull(


go-containerregistry also has pretty straightforward methods for pulling and pushing, which are already used in Tekton. Can we try to use those to make this easier to integrate down the line?

imjasonh · 2020-02-17T03:41:35Z

Cc @jonjohnsonjr

pierretasci

I think there is a case to be made here for exploring using go-containerregistry instead of oras if that is what Tekton already uses. Perhaps that can be done as a follow on exploration since this code as-is accomplishes the functionality intended.

pierretasci · 2020-02-17T16:23:29Z

oci/pkg/oci/bundle.go

+	// entities in a single file).
+	var entities []string
+	switch ext := path.Ext(filePath); true {
+	case ext == ".yaml" || ext == ".yml":


The UniversalDeserializer is being used here: https://github.com/tektoncd/experimental/pull/461/files/28fc8d52254c72a62a82d422db91ff4ecaf11b0a#diff-be720fcc306c3d06da4d0c6eac9ed71eR126 to decode the file contents and ensure they are parseable as Tekton crds.

The problem is that the deserializer, AFAIK, will only parse a single resource from a set of contents even if the file is a --- separated yaml config or a json array. We want to allow parsing multiple resources from a single file. If we take a look at how kubectl accomplishes this in the cli-runtime it is a bit more roundabout than what is trying to be accomplished here: https://github.com/kubernetes/kubectl/blob/4e4ec7cb5beac1fe75fd4be05d77c65030c30088/pkg/cmd/apply/apply.go#L323

To that end, for the first iteration of a tool that is meant just to be a POC, it might be worthwhile to keep the logic simple and more bespoke while we figure out how Tekton will support bundling these images. Perhaps we decide only Catalog CRDs should be bundled and we don't need parsing input at all"

pierretasci · 2020-02-17T16:24:41Z

oci/pkg/oci/image.go

+)
+
+func init() {
+	orascontext.GetLogger(context.Background()).Logger.SetLevel(logrus.ErrorLevel)


I believe this is actually just here to suppress the built-in logger of the oras package which spits out warnings for using a custom mime that isn't publicly registered anywhere. I don't think this PR specifies a logger and is, unfortunately, only importing logrus to set the log level.

pierretasci · 2020-02-17T16:30:09Z

oci/pkg/oci/bundle.go

+}
+
+// ReadPaths will recursively search each file path for Tekton resources and return the parsed specs or an error.
+func ReadPaths(filePaths []string) ([]ParsedTektonResource, error) {


I believe ReadPaths and ParsedTektonResource are the only exported members and they are referenced outside of the oci package inside of the action package. Perhaps there is a case to be made for re-organizing but this tends to follow a cobra cli layout as far as I can tell.

vdemeester · 2020-02-17T16:40:50Z

I think there is a case to be made here for exploring using go-containerregistry instead of oras if that is what Tekton already uses. Perhaps that can be done as a follow on exploration since this code as-is accomplishes the functionality intended.

I would definitely vote for that 👼 This is the experimental repository, it's meant to be experiments and to be simpler to get stuff in 👼

vdemeester · 2020-02-17T16:41:31Z

/cc @abayer @afrittoli @bobcatfish for a 2nd approval/lgtm 👼

imjasonh · 2020-02-17T19:55:40Z

/lgtm
/approve

+1 for getting it merged first as a POC, sorry if my comments made it seem like we needed to get those in ASAP. I can send some PRs to see what it looks like using go-containerregistry.

tekton-robot · 2020-02-17T19:55:45Z

[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~~ [vdemeester]

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

imjasonh · 2020-02-17T19:57:51Z

Re: supporting yamls with ---s I think it’d be fine to just prohibit those for the time being, and require inputs to be one per file. But again, we can discuss in a follow up PR, after this is merged.

tekton-robot requested review from abayer and dlorenc February 12, 2020 07:46

tekton-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. do-not-merge/invalid-owners-file Indicates that a PR should not merge because it has an invalid OWNERS file in it. labels Feb 12, 2020

sthaha added 3 commits February 12, 2020 22:25

Initial oci tool implementation by Pierre Tasci

7b0d3ad

This contains a POC of a cli that can push and pull tekton resources as OCI artifacts from a supported registry. Credits: Thanks to Pierre Tasci for this patch. Signed-off-by: Sunil Thaha <sthaha@redhat.com>

Reorg files to match tkn cli structure

b909754

Add Makefile similar to tkn

551025e

sthaha force-pushed the oci-tool branch from 0d61cef to 40a5caa Compare February 12, 2020 12:25

sthaha mentioned this pull request Feb 12, 2020

Feature: Versioning on Tasks/Pipelines tektoncd/pipeline#1839

Closed

vdemeester approved these changes Feb 12, 2020

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 12, 2020

Add ONWERS file

91da21f

sthaha force-pushed the oci-tool branch from 40a5caa to 91da21f Compare February 12, 2020 22:44

tekton-robot removed the do-not-merge/invalid-owners-file Indicates that a PR should not merge because it has an invalid OWNERS file in it. label Feb 12, 2020

git: ignore tmp and bin

43392e9

sthaha requested a review from bobcatfish February 17, 2020 00:57

sthaha added 2 commits February 17, 2020 11:31

Support multiple k8s resources in a single file

9eec0a5

Allow skipping of non Tekton resources

28fc8d5

imjasonh reviewed Feb 17, 2020

View reviewed changes

pierretasci reviewed Feb 17, 2020

View reviewed changes

tekton-robot assigned imjasonh Feb 17, 2020

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 17, 2020

tekton-robot merged commit fc335c0 into tektoncd:master Feb 17, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Oci tool: makes use of oci-artifacts to store and retrieve Tekton resources #461

Oci tool: makes use of oci-artifacts to store and retrieve Tekton resources #461

sthaha commented Feb 12, 2020

googlebot commented Feb 12, 2020

googlebot commented Feb 12, 2020

imjasonh Feb 17, 2020

imjasonh Feb 17, 2020

pierretasci Feb 17, 2020

imjasonh Feb 17, 2020

imjasonh Feb 17, 2020

pierretasci Feb 17, 2020 •

edited

Loading

imjasonh Feb 17, 2020

pierretasci Feb 17, 2020

imjasonh Feb 17, 2020

imjasonh commented Feb 17, 2020 •

edited

Loading

pierretasci left a comment

pierretasci Feb 17, 2020 •

edited

Loading

pierretasci Feb 17, 2020

pierretasci Feb 17, 2020

vdemeester commented Feb 17, 2020

vdemeester commented Feb 17, 2020

imjasonh commented Feb 17, 2020

tekton-robot commented Feb 17, 2020

imjasonh commented Feb 17, 2020

Oci tool: makes use of oci-artifacts to store and retrieve Tekton resources #461

Oci tool: makes use of oci-artifacts to store and retrieve Tekton resources #461

Conversation

sthaha commented Feb 12, 2020

Changes

Submitter Checklist

googlebot commented Feb 12, 2020

googlebot commented Feb 12, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pierretasci Feb 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imjasonh commented Feb 17, 2020 • edited Loading

pierretasci left a comment

Choose a reason for hiding this comment

pierretasci Feb 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vdemeester commented Feb 17, 2020

vdemeester commented Feb 17, 2020

imjasonh commented Feb 17, 2020

tekton-robot commented Feb 17, 2020

imjasonh commented Feb 17, 2020

pierretasci Feb 17, 2020 •

edited

Loading

imjasonh commented Feb 17, 2020 •

edited

Loading

pierretasci Feb 17, 2020 •

edited

Loading