Skip to content

Commit

Permalink
Remote Resolution Refactor
Browse files Browse the repository at this point in the history
This PR hides the implementation of the internally used API in an internal package. This allows us to update the internal interfaces freely without breaking users.

NOTE that this is a migration of what I think is only internally used. Our docs on using the resolver framework do not use any of these and neither do our built-in resolvers.
  • Loading branch information
chitrangpatel committed Apr 24, 2024
1 parent b419b2c commit c6d6698
Show file tree
Hide file tree
Showing 34 changed files with 89 additions and 89 deletions.
File renamed without changes.
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import (
"github.com/tektoncd/pipeline/pkg/apis/resolution/v1beta1"
rrclient "github.com/tektoncd/pipeline/pkg/client/resolution/clientset/versioned"
rrlisters "github.com/tektoncd/pipeline/pkg/client/resolution/listers/resolution/v1beta1"
resolutioncommon "github.com/tektoncd/pipeline/pkg/resolution/common"
"github.com/tektoncd/pipeline/pkg/resolution/common"
apierrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"knative.dev/pkg/apis"
Expand Down Expand Up @@ -63,7 +63,7 @@ func (r *CRDRequester) Submit(ctx context.Context, resolver ResolverName, req Re
!apierrors.IsAlreadyExists(err) {
return nil, err
}
return nil, resolutioncommon.ErrRequestInProgress
return nil, common.ErrRequestInProgress
}

if rr.Status.GetCondition(apis.ConditionSucceeded).IsUnknown() {
Expand All @@ -72,15 +72,15 @@ func (r *CRDRequester) Submit(ctx context.Context, resolver ResolverName, req Re
// that it doesn't get deleted until the caller is done
// with it. Use appendOwnerReference and then submit
// update to ResolutionRequest.
return nil, resolutioncommon.ErrRequestInProgress
return nil, common.ErrRequestInProgress
}

if rr.Status.GetCondition(apis.ConditionSucceeded).IsTrue() {
return crdIntoResource(rr), nil
}

message := rr.Status.GetCondition(apis.ConditionSucceeded).GetMessage()
err := resolutioncommon.NewError(resolutioncommon.ReasonResolutionFailed, errors.New(message))
err := common.NewError(common.ReasonResolutionFailed, errors.New(message))
return nil, err
}

Expand All @@ -94,7 +94,7 @@ func (r *CRDRequester) createResolutionRequest(ctx context.Context, resolver Res
Name: req.Name(),
Namespace: req.Namespace(),
Labels: map[string]string{
resolutioncommon.LabelKeyResolverType: string(resolver),
common.LabelKeyResolverType: string(resolver),
},
},
Spec: v1beta1.ResolutionRequestSpec{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,11 @@ import (
"testing"

"github.com/google/go-cmp/cmp"
resolutioncommon "github.com/tektoncd/pipeline/internal/resolution/common"
"github.com/tektoncd/pipeline/internal/resolution/resource"
"github.com/tektoncd/pipeline/pkg/apis/resolution/v1beta1"
ttesting "github.com/tektoncd/pipeline/pkg/reconciler/testing"
resolutioncommon "github.com/tektoncd/pipeline/pkg/resolution/common"
"github.com/tektoncd/pipeline/pkg/resolution/resource"
"github.com/tektoncd/pipeline/pkg/resolution/common"
"github.com/tektoncd/pipeline/test"
"github.com/tektoncd/pipeline/test/diff"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand Down Expand Up @@ -156,15 +157,15 @@ conditions:
inputResolutionRequest: nil,
expectedResolutionRequest: createdRR.DeepCopy(),
expectedResolvedResource: nil,
expectedErr: resolutioncommon.ErrRequestInProgress,
expectedErr: common.ErrRequestInProgress,
},
{
name: "resolution request exist and status is unknown",
inputRequest: request,
inputResolutionRequest: unknownRR.DeepCopy(),
expectedResolutionRequest: nil,
expectedResolvedResource: nil,
expectedErr: resolutioncommon.ErrRequestInProgress,
expectedErr: common.ErrRequestInProgress,
},
{
name: "resolution request exist and status is succeeded",
Expand All @@ -188,7 +189,7 @@ conditions:
inputResolutionRequest: failedRR.DeepCopy(),
expectedResolutionRequest: nil,
expectedResolvedResource: nil,
expectedErr: resolutioncommon.NewError(resolutioncommon.ReasonResolutionFailed, errors.New("error message")),
expectedErr: common.NewError(common.ReasonResolutionFailed, errors.New("error message")),
},
}

Expand Down
File renamed without changes.
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,8 @@ package resource_test
import (
"testing"

"github.com/tektoncd/pipeline/internal/resolution/resource"
v1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1"
"github.com/tektoncd/pipeline/pkg/resolution/resource"
)

func TestGenerateDeterministicName(t *testing.T) {
Expand Down
File renamed without changes.
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,8 @@ import (
"testing"

"github.com/google/go-cmp/cmp"
"github.com/tektoncd/pipeline/internal/resolution/resource"
v1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1"
"github.com/tektoncd/pipeline/pkg/resolution/resource"
"github.com/tektoncd/pipeline/test/diff"
)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ limitations under the License.
package resource

import (
"github.com/tektoncd/pipeline/pkg/resolution/common"
"github.com/tektoncd/pipeline/internal/resolution/common"
)

// This is an alias for avoiding cycle import
Expand Down
2 changes: 1 addition & 1 deletion pkg/reconciler/pipelinerun/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package pipelinerun
import (
"context"

resolution "github.com/tektoncd/pipeline/internal/resolution/resource"
"github.com/tektoncd/pipeline/pkg/apis/config"
"github.com/tektoncd/pipeline/pkg/apis/pipeline"
v1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1"
Expand All @@ -33,7 +34,6 @@ import (
"github.com/tektoncd/pipeline/pkg/pipelinerunmetrics"
cloudeventclient "github.com/tektoncd/pipeline/pkg/reconciler/events/cloudevent"
"github.com/tektoncd/pipeline/pkg/reconciler/volumeclaim"
resolution "github.com/tektoncd/pipeline/pkg/resolution/resource"
"github.com/tektoncd/pipeline/pkg/tracing"
"k8s.io/client-go/tools/cache"
"k8s.io/utils/clock"
Expand Down
2 changes: 1 addition & 1 deletion pkg/reconciler/pipelinerun/pipelinerun.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import (
"time"

"github.com/hashicorp/go-multierror"
resolution "github.com/tektoncd/pipeline/internal/resolution/resource"
"github.com/tektoncd/pipeline/pkg/apis/config"
"github.com/tektoncd/pipeline/pkg/apis/pipeline"
pipelineErrors "github.com/tektoncd/pipeline/pkg/apis/pipeline/errors"
Expand All @@ -52,7 +53,6 @@ import (
tresources "github.com/tektoncd/pipeline/pkg/reconciler/taskrun/resources"
"github.com/tektoncd/pipeline/pkg/reconciler/volumeclaim"
"github.com/tektoncd/pipeline/pkg/remote"
resolution "github.com/tektoncd/pipeline/pkg/resolution/resource"
"github.com/tektoncd/pipeline/pkg/substitution"
"github.com/tektoncd/pipeline/pkg/trustedresources"
"github.com/tektoncd/pipeline/pkg/workspace"
Expand Down
8 changes: 4 additions & 4 deletions pkg/reconciler/pipelinerun/pipelinerun_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import (
"github.com/google/go-cmp/cmp"
"github.com/google/go-cmp/cmp/cmpopts"
"github.com/sigstore/sigstore/pkg/signature"
remoteresource "github.com/tektoncd/pipeline/internal/resolution/resource"
"github.com/tektoncd/pipeline/pkg/apis/config"
cfgtesting "github.com/tektoncd/pipeline/pkg/apis/config/testing"
"github.com/tektoncd/pipeline/pkg/apis/pipeline"
Expand All @@ -48,8 +49,7 @@ import (
taskresources "github.com/tektoncd/pipeline/pkg/reconciler/taskrun/resources"
ttesting "github.com/tektoncd/pipeline/pkg/reconciler/testing"
"github.com/tektoncd/pipeline/pkg/reconciler/volumeclaim"
resolutioncommon "github.com/tektoncd/pipeline/pkg/resolution/common"
remoteresource "github.com/tektoncd/pipeline/pkg/resolution/resource"
"github.com/tektoncd/pipeline/pkg/resolution/common"
"github.com/tektoncd/pipeline/pkg/tracing"
"github.com/tektoncd/pipeline/pkg/trustedresources"
"github.com/tektoncd/pipeline/pkg/trustedresources/verifier"
Expand Down Expand Up @@ -8853,7 +8853,7 @@ spec:
t.Fatalf("expected resource request type %q but saw %q", resolutionRequestType, resolverName)
}

resreq.Status.MarkFailed(resolutioncommon.ReasonResolutionTimedOut, "resolution took longer than global timeout of 1 minute")
resreq.Status.MarkFailed(common.ReasonResolutionTimedOut, "resolution took longer than global timeout of 1 minute")
resreq, err = client.UpdateStatus(prt.TestAssets.Ctx, resreq, metav1.UpdateOptions{})
if err != nil {
t.Fatalf("unexpected error updating resource request with resolved pipeline data: %v", err)
Expand Down Expand Up @@ -8914,7 +8914,7 @@ spec:
t.Fatalf("expected resource request type %q but saw %q", resolutionRequestType, resolverName)
}

resreq.Status.MarkFailed(resolutioncommon.ReasonResolutionTimedOut, "resolution took longer than global timeout of 1 minute")
resreq.Status.MarkFailed(common.ReasonResolutionTimedOut, "resolution took longer than global timeout of 1 minute")
resreq, err = client.UpdateStatus(prt.TestAssets.Ctx, resreq, metav1.UpdateOptions{})
if err != nil {
t.Fatalf("unexpected error updating resource request with resolved pipeline data: %v", err)
Expand Down
2 changes: 1 addition & 1 deletion pkg/reconciler/pipelinerun/resources/pipelineref.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"errors"
"fmt"

remoteresource "github.com/tektoncd/pipeline/internal/resolution/resource"
v1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1"
"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1"
"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1"
Expand All @@ -29,7 +30,6 @@ import (
rprp "github.com/tektoncd/pipeline/pkg/reconciler/pipelinerun/pipelinespec"
"github.com/tektoncd/pipeline/pkg/remote"
"github.com/tektoncd/pipeline/pkg/remote/resolution"
remoteresource "github.com/tektoncd/pipeline/pkg/resolution/resource"
"github.com/tektoncd/pipeline/pkg/trustedresources"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
Expand Down
6 changes: 3 additions & 3 deletions pkg/reconciler/resolutionrequest/resolutionrequest.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import (

"github.com/tektoncd/pipeline/pkg/apis/resolution/v1beta1"
rrreconciler "github.com/tektoncd/pipeline/pkg/client/resolution/injection/reconciler/resolution/v1beta1/resolutionrequest"
resolutioncommon "github.com/tektoncd/pipeline/pkg/resolution/common"
"github.com/tektoncd/pipeline/pkg/resolution/common"
"k8s.io/utils/clock"
"knative.dev/pkg/apis"
"knative.dev/pkg/controller"
Expand Down Expand Up @@ -61,9 +61,9 @@ func (r *Reconciler) ReconcileKind(ctx context.Context, rr *v1beta1.ResolutionRe
case rr.Status.Data != "":
rr.Status.MarkSucceeded()
case requestDuration(rr) > defaultMaximumResolutionDuration:
rr.Status.MarkFailed(resolutioncommon.ReasonResolutionTimedOut, timeoutMessage())
rr.Status.MarkFailed(common.ReasonResolutionTimedOut, timeoutMessage())
default:
rr.Status.MarkInProgress(resolutioncommon.MessageWaitingForResolver)
rr.Status.MarkInProgress(common.MessageWaitingForResolver)
return controller.NewRequeueAfter(defaultMaximumResolutionDuration - requestDuration(rr))
}

Expand Down
8 changes: 4 additions & 4 deletions pkg/reconciler/resolutionrequest/resolutionrequest_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import (
"github.com/google/go-cmp/cmp/cmpopts"
"github.com/tektoncd/pipeline/pkg/apis/resolution/v1beta1"
ttesting "github.com/tektoncd/pipeline/pkg/reconciler/testing"
resolutioncommon "github.com/tektoncd/pipeline/pkg/resolution/common"
"github.com/tektoncd/pipeline/pkg/resolution/common"
"github.com/tektoncd/pipeline/test"
"github.com/tektoncd/pipeline/test/diff"
"github.com/tektoncd/pipeline/test/names"
Expand Down Expand Up @@ -106,8 +106,8 @@ func TestReconcile(t *testing.T) {
Conditions: duckv1.Conditions{{
Type: apis.ConditionSucceeded,
Status: corev1.ConditionUnknown,
Reason: resolutioncommon.ReasonResolutionInProgress,
Message: resolutioncommon.MessageWaitingForResolver,
Reason: common.ReasonResolutionInProgress,
Message: common.MessageWaitingForResolver,
}},
},
ResolutionRequestStatusFields: v1beta1.ResolutionRequestStatusFields{},
Expand All @@ -128,7 +128,7 @@ func TestReconcile(t *testing.T) {
Conditions: duckv1.Conditions{{
Type: apis.ConditionSucceeded,
Status: corev1.ConditionFalse,
Reason: resolutioncommon.ReasonResolutionTimedOut,
Reason: common.ReasonResolutionTimedOut,
Message: timeoutMessage(),
}},
},
Expand Down
2 changes: 1 addition & 1 deletion pkg/reconciler/taskrun/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package taskrun
import (
"context"

resolution "github.com/tektoncd/pipeline/internal/resolution/resource"
"github.com/tektoncd/pipeline/pkg/apis/config"
"github.com/tektoncd/pipeline/pkg/apis/pipeline"
v1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1"
Expand All @@ -31,7 +32,6 @@ import (
"github.com/tektoncd/pipeline/pkg/pod"
cloudeventclient "github.com/tektoncd/pipeline/pkg/reconciler/events/cloudevent"
"github.com/tektoncd/pipeline/pkg/reconciler/volumeclaim"
resolution "github.com/tektoncd/pipeline/pkg/resolution/resource"
"github.com/tektoncd/pipeline/pkg/spire"
"github.com/tektoncd/pipeline/pkg/taskrunmetrics"
"github.com/tektoncd/pipeline/pkg/tracing"
Expand Down
2 changes: 1 addition & 1 deletion pkg/reconciler/taskrun/resources/taskref.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,14 +22,14 @@ import (
"fmt"
"strings"

remoteresource "github.com/tektoncd/pipeline/internal/resolution/resource"
v1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1"
"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1"
"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1"
clientset "github.com/tektoncd/pipeline/pkg/client/clientset/versioned"
"github.com/tektoncd/pipeline/pkg/reconciler/apiserver"
"github.com/tektoncd/pipeline/pkg/remote"
"github.com/tektoncd/pipeline/pkg/remote/resolution"
remoteresource "github.com/tektoncd/pipeline/pkg/resolution/resource"
"github.com/tektoncd/pipeline/pkg/trustedresources"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
Expand Down
2 changes: 1 addition & 1 deletion pkg/reconciler/taskrun/resources/taskspec.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,11 @@ import (
"errors"
"fmt"

remoteresource "github.com/tektoncd/pipeline/internal/resolution/resource"
v1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1"
"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1"
clientset "github.com/tektoncd/pipeline/pkg/client/clientset/versioned"
resolutionutil "github.com/tektoncd/pipeline/pkg/internal/resolution"
remoteresource "github.com/tektoncd/pipeline/pkg/resolution/resource"
"github.com/tektoncd/pipeline/pkg/trustedresources"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/client-go/kubernetes"
Expand Down
2 changes: 1 addition & 1 deletion pkg/reconciler/taskrun/taskrun.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import (
"time"

"github.com/hashicorp/go-multierror"
resolution "github.com/tektoncd/pipeline/internal/resolution/resource"
"github.com/tektoncd/pipeline/internal/sidecarlogresults"
"github.com/tektoncd/pipeline/pkg/apis/config"
"github.com/tektoncd/pipeline/pkg/apis/pipeline"
Expand All @@ -47,7 +48,6 @@ import (
"github.com/tektoncd/pipeline/pkg/reconciler/taskrun/resources"
"github.com/tektoncd/pipeline/pkg/reconciler/volumeclaim"
"github.com/tektoncd/pipeline/pkg/remote"
resolution "github.com/tektoncd/pipeline/pkg/resolution/resource"
"github.com/tektoncd/pipeline/pkg/spire"
"github.com/tektoncd/pipeline/pkg/taskrunmetrics"
_ "github.com/tektoncd/pipeline/pkg/taskrunmetrics/fake" // Make sure the taskrunmetrics are setup
Expand Down
6 changes: 3 additions & 3 deletions pkg/reconciler/taskrun/taskrun_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ import (
"github.com/google/go-cmp/cmp/cmpopts"
"github.com/google/go-containerregistry/pkg/registry"
"github.com/sigstore/sigstore/pkg/signature"
remoteresource "github.com/tektoncd/pipeline/internal/resolution/resource"
"github.com/tektoncd/pipeline/pkg/apis/config"
cfgtesting "github.com/tektoncd/pipeline/pkg/apis/config/testing"
"github.com/tektoncd/pipeline/pkg/apis/pipeline"
Expand All @@ -53,8 +54,7 @@ import (
"github.com/tektoncd/pipeline/pkg/reconciler/taskrun/resources"
ttesting "github.com/tektoncd/pipeline/pkg/reconciler/testing"
"github.com/tektoncd/pipeline/pkg/reconciler/volumeclaim"
resolutioncommon "github.com/tektoncd/pipeline/pkg/resolution/common"
remoteresource "github.com/tektoncd/pipeline/pkg/resolution/resource"
"github.com/tektoncd/pipeline/pkg/resolution/common"
"github.com/tektoncd/pipeline/pkg/trustedresources"
"github.com/tektoncd/pipeline/pkg/trustedresources/verifier"
"github.com/tektoncd/pipeline/pkg/workspace"
Expand Down Expand Up @@ -1285,7 +1285,7 @@ spec:
t.Fatalf("expected resource request type %q but saw %q", resolutionRequestType, resolverName)
}

resreq.Status.MarkFailed(resolutioncommon.ReasonResolutionTimedOut, "resolution took longer than global timeout of 1 minute")
resreq.Status.MarkFailed(common.ReasonResolutionTimedOut, "resolution took longer than global timeout of 1 minute")
resreq, err = client.UpdateStatus(testAssets.Ctx, resreq, metav1.UpdateOptions{})
if err != nil {
t.Fatalf("unexpected error updating resource request with resolved pipeline data: %v", err)
Expand Down
2 changes: 1 addition & 1 deletion pkg/remote/resolution/request.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ limitations under the License.
package resolution

import (
resolution "github.com/tektoncd/pipeline/pkg/resolution/resource"
resolution "github.com/tektoncd/pipeline/internal/resolution/resource"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"knative.dev/pkg/kmeta"
)
Expand Down
6 changes: 3 additions & 3 deletions pkg/remote/resolution/resolver.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,11 @@ import (
"errors"
"fmt"

remoteresource "github.com/tektoncd/pipeline/internal/resolution/resource"
v1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1"
"github.com/tektoncd/pipeline/pkg/client/clientset/versioned/scheme"
"github.com/tektoncd/pipeline/pkg/remote"
resolutioncommon "github.com/tektoncd/pipeline/pkg/resolution/common"
remoteresource "github.com/tektoncd/pipeline/pkg/resolution/resource"
"github.com/tektoncd/pipeline/pkg/resolution/common"
"k8s.io/apimachinery/pkg/runtime"
"knative.dev/pkg/kmeta"
)
Expand Down Expand Up @@ -64,7 +64,7 @@ func (resolver *Resolver) Get(ctx context.Context, _, _ string) (runtime.Object,
}
resolved, err := resolver.requester.Submit(ctx, resolverName, req)
switch {
case errors.Is(err, resolutioncommon.ErrRequestInProgress):
case errors.Is(err, common.ErrRequestInProgress):
return nil, nil, remote.ErrRequestInProgress
case err != nil:
return nil, nil, fmt.Errorf("error requesting remote resource: %w", err)
Expand Down
6 changes: 3 additions & 3 deletions pkg/remote/resolution/resolver_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,10 @@ import (
"testing"

"github.com/google/go-cmp/cmp"
remoteresource "github.com/tektoncd/pipeline/internal/resolution/resource"
"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1"
"github.com/tektoncd/pipeline/pkg/remote"
resolutioncommon "github.com/tektoncd/pipeline/pkg/resolution/common"
remoteresource "github.com/tektoncd/pipeline/pkg/resolution/resource"
"github.com/tektoncd/pipeline/pkg/resolution/common"
"github.com/tektoncd/pipeline/test"
"github.com/tektoncd/pipeline/test/diff"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand Down Expand Up @@ -90,7 +90,7 @@ func TestGet_Errors(t *testing.T) {
expectedGetErr error
resolvedResource remoteresource.ResolvedResource
}{{
submitErr: resolutioncommon.ErrRequestInProgress,
submitErr: common.ErrRequestInProgress,
expectedGetErr: remote.ErrRequestInProgress,
resolvedResource: nil,
}, {
Expand Down
Loading

0 comments on commit c6d6698

Please sign in to comment.