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.

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

Refactor to the remote image library and task references #2395

Merged
merged 1 commit into from
Apr 28, 2020

Conversation

sthaha
Copy link
Member

@sthaha sthaha commented Apr 14, 2020

This change is part of larger effort to support OCI image references in
task and pipeline refs. This intial change is a refactor of existing
logic for clarity and in preparation of adding the new API fields into
the Tekton objects.

For more details see the design: https://docs.google.com/document/d/1lXF_SvLwl6OqqGy8JbpSXRj4hWJ6CSImlxlIl4V9rnM/edit#

Principle changes:

  • Refactor the remote library to operate more like an independent API.
    It allows the user to Get and List objects in a remote image. Decoupling
    the API of fetching and inspecting images allows this to be reused by
    external clients (like tkn).
  • Create a new taskref library resolver that knows how to resolve local
    references. This mirrors the remote resolver and now establishes an
    ability for the next PR to create a factory function that assigns the
    correct resolver based on the TaskRun. It also allows us to test this
    logic by putting it into a function.
  • Likewise, we have created a new pipelineref library for all the same reasons above.

Signed-off-by: Sunil Thaha sthaha@redhat.com

Changes

Submitter Checklist

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

See the contribution guide for more details.

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

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

Describe any user facing changes here, or delete this block.

Examples of user facing changes:
- API changes
- Bug fixes
- Any changes in behavior

@tekton-robot tekton-robot requested review from abayer and dlorenc April 14, 2020 23:03
@googlebot googlebot added the cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit label Apr 14, 2020
@tekton-robot tekton-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Apr 14, 2020
@tekton-robot
Copy link
Collaborator

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
test/builder/task.go 68.2% 68.2% -0.0

@sthaha
Copy link
Member Author

sthaha commented Apr 14, 2020

/test tekton-pipeline-unit-tests

@sthaha sthaha force-pushed the refactor-remote-img-pkg branch from 228f67c to 08933e4 Compare April 15, 2020 00:36
@tekton-robot
Copy link
Collaborator

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
test/builder/task.go 68.2% 68.2% -0.0

// CreateImage will push a new OCI image artifact with the provided raw data object as a layer and return the full image
// reference with a digest to fetch the image. Key must be specified as [lowercase kind]/[object name]. The image ref
// with a digest is returned.
func CreateImage(ref string, objs ...runtime.Object) (string, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

The goal is to abandon the experimental tool so we can go in and remove it.

Copy link
Member

Choose a reason for hiding this comment

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

The experimental tool will still have value for a while, to be able to test this feature we will need to be able to create such images, having the oci tool in experimental is the best way to achieve that while we work on this.

To re-iterate a little bit what tektoncd/experimental is for : quicker dev to in master cycle. It's way easier to iterate quickly in experimental, demo it and later on integrating it in the other projects (here, on cli, …)

Choose a reason for hiding this comment

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

Discussed offline. Our hope is to get in a "spec" document in the next PR to act as the stopgap so users will be able to create their own solution while waiting for the official Tekton solution.

var gtFunc resources.GetTask
// TODO: Use factory func instead of hard-coding this once OCI images are supported.
func (c *Reconciler) getTaskResolver(tr *v1alpha1.TaskRun) (*resources.LocalTaskRefResolver, v1alpha1.TaskKind) {
resolver := &resources.LocalTaskRefResolver{
Copy link
Member

Choose a reason for hiding this comment

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

Why not creating the resolver after the if ? the you could inline all.

resolver := &resources.LocalTaskRefResolver{
	Namespace:    tr.Namespace,
	Tektonclient: c.PipelineClientSet,
	Kind: kind,
}

Choose a reason for hiding this comment

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

True. This function is actually being deleted in the next PR. Both are very long and do different things so we wanted to split them up. We can still inline it if you want or we can wait until the next one.

// Resolver defines a generic API to retrieve Tekton resources from remote locations. It allows 2 principle operations:
// - List: retrieve a flat set of Tekton objects in this remote location
// - Get: retrieves a specific object with the given Kind and name.
type Resolver interface {
Copy link
Member

Choose a reason for hiding this comment

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

This interface would be better defined where we use it (and we might not even need to be exported in that case).
One reason for this is the dependencies it brings if we want to use it — aka any package the pkg/remote package depends on.

This means at least:

	"github.com/google/go-containerregistry/pkg/authn"
	imgname "github.com/google/go-containerregistry/pkg/name"
	v1 "github.com/google/go-containerregistry/pkg/v1"
	"github.com/google/go-containerregistry/pkg/v1/remote"
	"github.com/tektoncd/pipeline/pkg/client/clientset/versioned/scheme"
	"k8s.io/apimachinery/pkg/runtime"
  • Using this interface shouldn't require any dependency on go-containerregistry (using an impl. might).
  • I wonder about the runtime.Object return case too 🤔

Choose a reason for hiding this comment

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

Re: runtime.Object it was the most generic, universal object I could think of to return either a Pipeline, Task, or ClusterTask out of the same API.

On the larger question of where to put the interface. I was thinking it would make sense to co-locate it with the definitions since that is where users would expect it to be. You are correct that it would require bringing in the container dependencies and if we add a git implementation, git dependencies as well. I guess we could put the resolver interface under the remote package to centralize it and then create sub-packages with each implementation so that you only import the ones you care about? Does that make sense?

Copy link
Member

Choose a reason for hiding this comment

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

On the larger question of where to put the interface. I was thinking it would make sense to co-locate it with the definitions since that is where users would expect it to be. You are correct that it would require bringing in the container dependencies and if we add a git implementation, git dependencies as well. I guess we could put the resolver interface under the remote package to centralize it and then create sub-packages with each implementation so that you only import the ones you care about? Does that make sense?

I would work — it's not the best from my point of view (aka I am not a huge fan of exported interface), but I think it's a good start 😉

Choose a reason for hiding this comment

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

Done

@imjasonh
Copy link
Member

/assign @imjasonh

@vdemeester
Copy link
Member

Quick question @sthaha @pierretasci, how does it relates to

If it carry / override those previous PRs, we should close the previous PR 👼 🙏

@pierretasci
Copy link

Quick question @sthaha @pierretasci, how does it relates to

If it carry / override those previous PRs, we should close the previous PR 👼 🙏

You are correct, those are abandoned for this new path. I will close them.

@sthaha sthaha force-pushed the refactor-remote-img-pkg branch 2 times, most recently from ee364c0 to 28d889c Compare April 20, 2020 01:48
@tekton-robot
Copy link
Collaborator

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
test/builder/task.go 67.7% 68.2% 0.4

@pierretasci
Copy link

  • friendly ping * this is ready for re-review

Copy link
Member

@imjasonh imjasonh left a comment

Choose a reason for hiding this comment

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

/lgtm

if actual == nil {
t.Errorf("Expected a PipelineRun to be updated, but it wasn't.")
}
t.Log(clients.Pipeline.Actions())
Copy link
Member

Choose a reason for hiding this comment

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

Is this logging statement necessary?

Choose a reason for hiding this comment

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

You're right. This was a debug that made it in. Removing.

import (
"fmt"

"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1"
Copy link
Member

Choose a reason for hiding this comment

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

Will this still work if the PipelineRun is a v1beta1 PipelineRun object?

Choose a reason for hiding this comment

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

The current functionality is to always return it as a v1alpha1.PipelineInterface so that the existing reconciler logic to upconvert it to v1beta1 is maintained. Once we move all of the internals to v1beta1 we can probably move to that or split this api.

Copy link
Member

Choose a reason for hiding this comment

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

Right, with #2413 and #2410, we will be able to use v1beta1 everywhere 😉

// return an error if it can't find an appropriate Task for any reason.
func (l *LocalTaskRefResolver) GetTask(name string) (v1alpha1.TaskInterface, error) {
if l.Kind == v1alpha1.ClusterTaskKind {
task, err := l.Tektonclient.TektonV1alpha1().ClusterTasks().Get(name, metav1.GetOptions{})
Copy link
Member

Choose a reason for hiding this comment

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

style nit: This can just be return l.Tektonclient...ClusterTasks().Get(...)

Choose a reason for hiding this comment

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

👍

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 22, 2020
@sthaha sthaha force-pushed the refactor-remote-img-pkg branch from 28d889c to 053610b Compare April 22, 2020 22:09
@tekton-robot tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Apr 22, 2020
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Apr 22, 2020

CLA Check
The committers are authorized under a signed CLA.

@tekton-robot
Copy link
Collaborator

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
test/builder/task.go 68.2% 68.6% 0.4

@imjasonh
Copy link
Member

/lgtm

I'll leave it to @vdemeester to approve

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 23, 2020
Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

/meow

Let's do the rest in follow-ups 😉

@tekton-robot
Copy link
Collaborator

@vdemeester: cat image

In response to this:

/meow

Let's do the rest in follow-ups 😉

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@tekton-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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:

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

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 27, 2020
@vdemeester
Copy link
Member

arf, needs a rebase @sthaha @pierretasci 😅

This change is part of larger effort to support OCI image references in
task and pipeline refs. This intial change is a refactor of existing
logic for clarity and in preparation of adding the new API fields into
the Tekton objects.

For more details see the design: https://docs.google.com/document/d/1lXF_SvLwl6OqqGy8JbpSXRj4hWJ6CSImlxlIl4V9rnM/edit#

Principle changes:
- Refactor the remote library to operate more like an independent API.
  It allows the user to Get and List objects in a remote image. Decoupling
  the API of fetching and inspecting images allows this to be reused by
  external clients (like `tkn`).
- Create a new taskref library resolver that knows how to resolve local
  references. This mirrors the remote resolver and now establishes an
  ability for the next PR to create a factory function that assigns the
  correct resolver based on the TaskRun. It also allows us to test this
  logic by putting it into a function.
- Likewise, we have created a new pipelineref library for all the same reasons above.

Signed-off-by: Sunil Thaha <sthaha@redhat.com>
@sthaha sthaha force-pushed the refactor-remote-img-pkg branch from 053610b to 8bca87e Compare April 28, 2020 02:39
@tekton-robot tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Apr 28, 2020
@tekton-robot
Copy link
Collaborator

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

File Old Coverage New Coverage Delta
pkg/reconciler/pipelinerun/pipelinerun.go 72.6% 72.2% -0.5
pkg/reconciler/pipelinerun/resources/pipelineref.go Do not exist 66.7%
pkg/reconciler/taskrun/resources/taskref.go Do not exist 75.0%
pkg/reconciler/taskrun/taskrun.go 75.2% 74.5% -0.7
pkg/remote/oci/resolver.go Do not exist 76.9%
test/builder/task.go 68.2% 68.6% 0.4
test/remote.go Do not exist 71.4%

@vdemeester
Copy link
Member

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 28, 2020
@tekton-robot tekton-robot merged commit b435bd9 into tektoncd:master Apr 28, 2020
@afrittoli afrittoli added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label Apr 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants