Skip to content

Commit

Permalink
refactor
Browse files Browse the repository at this point in the history
  • Loading branch information
@Yongxuanzhang
Yongxuanzhang committed Nov 1, 2023
1 parent f37e1b6 commit c79c103
Show file tree
Hide file tree
Showing 3 changed files with 57 additions and 54 deletions.
13 changes: 3 additions & 10 deletions pkg/reconciler/taskrun/resources/taskspec.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ type GetTaskRun func(string) (*v1.TaskRun, error)
// GetTaskData will retrieve the Task metadata and Spec associated with the
// provided TaskRun. This can come from a reference Task or from the TaskRun's
// metadata and embedded TaskSpec.
func GetTaskData(ctx context.Context, taskRun *v1.TaskRun, getTask GetTask, tekton clientset.Interface, k8s kubernetes.Interface, requester remoteresource.Requester) (*resolutionutil.ResolvedObjectMeta, *v1.TaskSpec, error) {
func GetTaskData(ctx context.Context, taskRun *v1.TaskRun, getTask GetTask) (*resolutionutil.ResolvedObjectMeta, *v1.TaskSpec, error) {
taskMeta := metav1.ObjectMeta{}
taskSpec := v1.TaskSpec{}
var refSource *v1.RefSource
Expand Down Expand Up @@ -92,13 +92,6 @@ func GetTaskData(ctx context.Context, taskRun *v1.TaskRun, getTask GetTask, tekt
return nil, nil, fmt.Errorf("taskRun %s not providing TaskRef or TaskSpec", taskRun.Name)
}

steps, err := extractStepActions(ctx, taskSpec, taskRun, tekton, k8s, requester)
if err != nil {
return nil, nil, err
} else {
taskSpec.Steps = steps
}

taskSpec.SetDefaults(ctx)
return &resolutionutil.ResolvedObjectMeta{
ObjectMeta: &taskMeta,
Expand All @@ -107,8 +100,8 @@ func GetTaskData(ctx context.Context, taskRun *v1.TaskRun, getTask GetTask, tekt
}, &taskSpec, nil
}

// extractStepActions extracts the StepActions and merges them with the inlined Step specification.
func extractStepActions(ctx context.Context, taskSpec v1.TaskSpec, taskRun *v1.TaskRun, tekton clientset.Interface, k8s kubernetes.Interface, requester remoteresource.Requester) ([]v1.Step, error) {
// GetStepActionsData extracts the StepActions and merges them with the inlined Step specification.
func GetStepActionsData(ctx context.Context, taskSpec v1.TaskSpec, taskRun *v1.TaskRun, tekton clientset.Interface, k8s kubernetes.Interface, requester remoteresource.Requester) ([]v1.Step, error) {
steps := []v1.Step{}
for _, step := range taskSpec.Steps {
s := step.DeepCopy()
Expand Down
73 changes: 30 additions & 43 deletions pkg/reconciler/taskrun/resources/taskspec_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ func TestGetTaskSpec_Ref(t *testing.T) {
gt := func(ctx context.Context, n string) (*v1.Task, *v1.RefSource, *trustedresources.VerificationResult, error) {
return task, sampleRefSource.DeepCopy(), nil, nil
}
resolvedObjectMeta, taskSpec, err := resources.GetTaskData(context.Background(), tr, gt, nil, nil, nil)
resolvedObjectMeta, taskSpec, err := resources.GetTaskData(context.Background(), tr, gt)

if err != nil {
t.Fatalf("Did not expect error getting task spec but got: %s", err)
Expand Down Expand Up @@ -94,7 +94,7 @@ func TestGetTaskSpec_Embedded(t *testing.T) {
gt := func(ctx context.Context, n string) (*v1.Task, *v1.RefSource, *trustedresources.VerificationResult, error) {
return nil, nil, nil, errors.New("shouldn't be called")
}
resolvedObjectMeta, taskSpec, err := resources.GetTaskData(context.Background(), tr, gt, nil, nil, nil)
resolvedObjectMeta, taskSpec, err := resources.GetTaskData(context.Background(), tr, gt)

if err != nil {
t.Fatalf("Did not expect error getting task spec but got: %s", err)
Expand Down Expand Up @@ -123,7 +123,7 @@ func TestGetTaskSpec_Invalid(t *testing.T) {
gt := func(ctx context.Context, n string) (*v1.Task, *v1.RefSource, *trustedresources.VerificationResult, error) {
return nil, nil, nil, errors.New("shouldn't be called")
}
_, _, err := resources.GetTaskData(context.Background(), tr, gt, nil, nil, nil)
_, _, err := resources.GetTaskData(context.Background(), tr, gt)
if err == nil {
t.Fatalf("Expected error resolving spec with no embedded or referenced task spec but didn't get error")
}
Expand All @@ -143,7 +143,7 @@ func TestGetTaskSpec_Error(t *testing.T) {
gt := func(ctx context.Context, n string) (*v1.Task, *v1.RefSource, *trustedresources.VerificationResult, error) {
return nil, nil, nil, errors.New("something went wrong")
}
_, _, err := resources.GetTaskData(context.Background(), tr, gt, nil, nil, nil)
_, _, err := resources.GetTaskData(context.Background(), tr, gt)
if err == nil {
t.Fatalf("Expected error when unable to find referenced Task but got none")
}
Expand Down Expand Up @@ -187,7 +187,7 @@ func TestGetTaskData_ResolutionSuccess(t *testing.T) {
}, sampleRefSource.DeepCopy(), nil, nil
}
ctx := context.Background()
resolvedMeta, resolvedSpec, err := resources.GetTaskData(ctx, tr, getTask, nil, nil, nil)
resolvedMeta, resolvedSpec, err := resources.GetTaskData(ctx, tr, getTask)
if err != nil {
t.Fatalf("Unexpected error getting mocked data: %v", err)
}
Expand Down Expand Up @@ -221,7 +221,7 @@ func TestGetPipelineData_ResolutionError(t *testing.T) {
return nil, nil, nil, errors.New("something went wrong")
}
ctx := context.Background()
_, _, err := resources.GetTaskData(ctx, tr, getTask, nil, nil, nil)
_, _, err := resources.GetTaskData(ctx, tr, getTask)
if err == nil {
t.Fatalf("Expected error when unable to find referenced Task but got none")
}
Expand All @@ -244,7 +244,7 @@ func TestGetTaskData_ResolvedNilTask(t *testing.T) {
return nil, nil, nil, nil
}
ctx := context.Background()
_, _, err := resources.GetTaskData(ctx, tr, getTask, nil, nil, nil)
_, _, err := resources.GetTaskData(ctx, tr, getTask)
if err == nil {
t.Fatalf("Expected error when unable to find referenced Task but got none")
}
Expand Down Expand Up @@ -291,7 +291,7 @@ func TestGetTaskData_VerificationResult(t *testing.T) {
Spec: *sourceSpec.DeepCopy(),
}, nil, verificationResult, nil
}
r, _, err := resources.GetTaskData(context.Background(), tr, getTask, nil, nil, nil)
r, _, err := resources.GetTaskData(context.Background(), tr, getTask)
if err != nil {
t.Fatalf("Did not expect error but got: %s", err)
}
Expand All @@ -300,11 +300,11 @@ func TestGetTaskData_VerificationResult(t *testing.T) {
}
}

func TestGetStepAction(t *testing.T) {
func TestGetStepActionsData(t *testing.T) {
tests := []struct {
name string
tr *v1.TaskRun
want *v1.TaskSpec
want []v1.Step
stepAction *v1alpha1.StepAction
}{{
name: "step-action-with-command-args",
Expand All @@ -325,15 +325,13 @@ func TestGetStepAction(t *testing.T) {
},
},
},
want: &v1.TaskSpec{
Steps: []v1.Step{{
Image: "myimage",
Command: []string{"ls"},
Args: []string{"-lh"},
WorkingDir: "/bar",
Timeout: &metav1.Duration{Duration: time.Hour},
}},
},
want: []v1.Step{{
Image: "myimage",
Command: []string{"ls"},
Args: []string{"-lh"},
WorkingDir: "/bar",
Timeout: &metav1.Duration{Duration: time.Hour},
}},
stepAction: &v1alpha1.StepAction{
ObjectMeta: metav1.ObjectMeta{
Name: "stepAction",
Expand Down Expand Up @@ -362,12 +360,10 @@ func TestGetStepAction(t *testing.T) {
},
},
},
want: &v1.TaskSpec{
Steps: []v1.Step{{
Image: "myimage",
Script: "ls",
}},
},
want: []v1.Step{{
Image: "myimage",
Script: "ls",
}},
stepAction: &v1alpha1.StepAction{
ObjectMeta: metav1.ObjectMeta{
Name: "stepActionWithScript",
Expand Down Expand Up @@ -395,15 +391,13 @@ func TestGetStepAction(t *testing.T) {
},
},
},
want: &v1.TaskSpec{
Steps: []v1.Step{{
Image: "myimage",
Env: []corev1.EnvVar{{
Name: "env1",
Value: "value1",
}},
want: []v1.Step{{
Image: "myimage",
Env: []corev1.EnvVar{{
Name: "env1",
Value: "value1",
}},
},
}},
stepAction: &v1alpha1.StepAction{
ObjectMeta: metav1.ObjectMeta{
Name: "stepActionWithEnv",
Expand All @@ -419,13 +413,10 @@ func TestGetStepAction(t *testing.T) {
},
}}
for _, tt := range tests {
gt := func(ctx context.Context, n string) (*v1.Task, *v1.RefSource, *trustedresources.VerificationResult, error) {
return nil, nil, nil, nil
}
ctx := context.Background()
tektonclient := fake.NewSimpleClientset(tt.stepAction)

_, got, err := resources.GetTaskData(ctx, tt.tr, gt, tektonclient, nil, nil)
got, err := resources.GetStepActionsData(ctx, *tt.tr.Spec.TaskSpec, tt.tr, tektonclient, nil, nil)
if err != nil {
t.Errorf("Did not expect an error but got : %s", err)
}
Expand All @@ -435,7 +426,7 @@ func TestGetStepAction(t *testing.T) {
}
}

func TestGetStepAction_Error(t *testing.T) {
func TestGetStepActionsData_Error(t *testing.T) {
tests := []struct {
name string
tr *v1.TaskRun
Expand All @@ -461,11 +452,7 @@ func TestGetStepAction_Error(t *testing.T) {
expectedError: fmt.Errorf("must specify namespace to resolve reference to step action stepActionError"),
}}
for _, tt := range tests {
gt := func(ctx context.Context, n string) (*v1.Task, *v1.RefSource, *trustedresources.VerificationResult, error) {
return nil, nil, nil, nil
}

_, _, err := resources.GetTaskData(context.Background(), tt.tr, gt, nil, nil, nil)
_, err := resources.GetStepActionsData(context.Background(), *tt.tr.Spec.TaskSpec, tt.tr, nil, nil, nil)
if err == nil {
t.Fatalf("Expected to get an error but did not find any.")
}
Expand Down
25 changes: 24 additions & 1 deletion pkg/reconciler/taskrun/taskrun.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -333,7 +333,7 @@ func (c *Reconciler) prepare(ctx context.Context, tr *v1.TaskRun) (*v1.TaskSpec,
}
getTaskfunc := resources.GetTaskFuncFromTaskRun(ctx, c.KubeClientSet, c.PipelineClientSet, c.resolutionRequester, tr, vp)

taskMeta, taskSpec, err := resources.GetTaskData(ctx, tr, getTaskfunc, c.PipelineClientSet, c.KubeClientSet, c.resolutionRequester)
taskMeta, taskSpec, err := resources.GetTaskData(ctx, tr, getTaskfunc)
switch {
case errors.Is(err, remote.ErrRequestInProgress):
message := fmt.Sprintf("TaskRun %s/%s awaiting remote resource", tr.Namespace, tr.Name)
Expand All @@ -358,6 +358,29 @@ func (c *Reconciler) prepare(ctx context.Context, tr *v1.TaskRun) (*v1.TaskSpec,
}
}

steps, err := resources.GetStepActionsData(ctx, *taskSpec, tr, c.PipelineClientSet, c.KubeClientSet, c.resolutionRequester)
switch {
case errors.Is(err, remote.ErrRequestInProgress):
message := fmt.Sprintf("TaskRun %s/%s awaiting remote StepAction", tr.Namespace, tr.Name)
tr.Status.MarkResourceOngoing(v1.TaskRunReasonResolvingTaskRef, message)
return nil, nil, err
case errors.Is(err, apiserver.ErrReferencedObjectValidationFailed), errors.Is(err, apiserver.ErrCouldntValidateObjectPermanent):
tr.Status.MarkResourceFailed(podconvert.ReasonTaskFailedValidation, err)
return nil, nil, controller.NewPermanentError(err)
case errors.Is(err, apiserver.ErrCouldntValidateObjectRetryable):
return nil, nil, err
case err != nil:
logger.Errorf("Failed to determine StepAction to use for taskrun %s: %v", tr.Name, err)
if resources.IsGetTaskErrTransient(err) {
return nil, nil, err
}
tr.Status.MarkResourceFailed(podconvert.ReasonFailedResolution, err)
return nil, nil, controller.NewPermanentError(err)
default:
// Store the fetched StepActions to taskspec
taskSpec.Steps = steps
}

if taskMeta.VerificationResult != nil {
switch taskMeta.VerificationResult.VerificationResultType {
case trustedresources.VerificationError:
Expand Down

0 comments on commit c79c103

Please sign in to comment.