Remove runtime dependency on Build controller #326
Conversation
…n't work otherwise
There was a problem hiding this comment.
Great set of changes 👍
Heads up about changes in output design PR. Taskrun invokes input_resouces and output_resources functions to add steps before and after build steps like a wrapper. If that PR gets merged then portion of code in taskrun will have conflicts
hack/release.sh
Outdated
| run_validation_tests ./test/presubmit-tests.sh | ||
|
|
||
| # Build the release | ||
|
|
||
| banner "Building the release" | ||
|
|
||
| # Location of the base image for creds-init and git images | ||
| readonly BUILD_BASE_GCR="${KO_DOCKER_REPO}/github.com/knative/build-pipeline/build-base" |
There was a problem hiding this comment.
nit: Can we renameBUILD_BASE_GCR to PIPELINE_BASE_GCR ?
| @@ -175,10 +175,10 @@ type TaskRunList struct { | |||
| } | |||
|
|
|||
| // GetBuildRef for task | |||
| func (tr *TaskRun) GetBuildRef() corev1.ObjectReference { | |||
| func (tr *TaskRun) GetBuildPodRef() corev1.ObjectReference { | |||
There was a problem hiding this comment.
nit: comment for the function is referring to old function name
| // Rewrite the pod's OwnerRef to point the TaskRun, instead of a | ||
| // non-existent build. | ||
| pod.OwnerReferences = []metav1.OwnerReference{ | ||
| *metav1.NewControllerRef(tr, schema.GroupVersionKind{ |
There was a problem hiding this comment.
There is a variable called groupVersionKind which hold this value so this might become simple one liner
*metav1.NewControllerRef(tr, groupVersionKind)
|
/test pull-knative-build-pipeline-go-coverage |
| import "fmt" | ||
|
|
||
| func main() { | ||
| fmt.Println("Build successful") |
There was a problem hiding this comment.
Suggestion:
In output PR, I am introducing shell based image. We could consider using that image and pass arguments echo Build Successful instead of maintaining nop image
There was a problem hiding this comment.
Maybe. Having this image based on distroless/base means that the image doesn't have a shell at all, which reduces its size and attack surface. I'm not opposed if this becomes difficult to maintain for some reason, but we should bias toward minimal images where possible.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ImJasonH, shashwathi 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 |
|
The following is the coverage report on pkg/.
|
|
Great PR, this will be a huge step for moving the pipeline crd project forward 👍🚀 /lgtm |
Syncs nop image as per new ci
With this change,
TaskRuns are translated toBuilds in the TaskRun controller (as today), then translated fromBuilds toPods using the same vendored code from theknative/buildrepo that the Build controller uses.The TaskRun controller then watches Pods for updates, instead of Builds, and when it sees a Pod update, it translates that PodStatus to a BuildStatus (as the Build controller does today), then translates the BuildStatus to a TaskRunStatus (as the Pod controller does today).
As a result, there is no runtime dependency on the Build controller, and no Build CRD resources are created at any point.
This is Phase 1 below:
As a next step, we can remove the code dependency on the Build type, and the duplicate type translation, by simply translating
TaskRuns directly toPods, andPodStatusdirectly toTaskRunStatus.