Change podconvert.MakePod func into a configuration struct with methods #2982
Conversation
tekton-robot
added
release-note-none
|
The following is the coverage report on the affected files.
|
|
/kind cleanup |
tekton-robot
added
the
kind/cleanup
| @@ -1760,7 +1760,13 @@ func makePod(taskRun *v1beta1.TaskRun, task *v1beta1.Task) (*corev1.Pod, error) | |||
| return nil, err | |||
| } | |||
|
|
|||
| return podconvert.MakePod(context.Background(), images, taskRun, task.Spec, kubeclient, entrypointCache, true) | |||
| builder := podconvert.PodBuilder{ | |||
There was a problem hiding this comment.
nit: it bothers me it's not named the same as above 😹 but that's definitely a nit 😂
There was a problem hiding this comment.
hm, i'm open to changing the naming, no big deal, but i'm not 100% clear what name you want - right now it's podconvert.Builder after golangci-lint corrected me. What's your preferred name?
There was a problem hiding this comment.
I was speaker about the variable name builder (and above podBuilder) 😊
While working on a feature to optionally disable Tekton's built-in credentials initialization process, it became apparent that the podconvert.MakePod func would need an extra boolean argument in its signature. The signature is already quite long which makes reading it difficult. This commit separates the signature into configuration options and the task spec / run. In a follow-up commit I plan to add the boolean flag described above as another configuration option in the PodBuilder struct.
|
The following is the coverage report on the affected files.
|
|
I went back and forth in my head twice on whether or not the kubernetes client should be on this struct vs. left as a function paramter but think I settled on the way you did it here. Nice! /lgtm |
|
/lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dlorenc 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 |
tekton-robot
added
the
approved
Changes
While working on a feature to optionally disable Tekton's built-in credentials
initialization process, it became apparent that the podconvert.MakePod func
would need an extra boolean argument in its signature. The signature is already
quite long which makes reading it difficult.
This commit separates the signature into configuration options and the task
spec / run arguments. In a follow-up commit I plan to add the boolean flag described
above as another configuration option in the PodBuilder struct.
Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
Release Notes