BUILD-476: Tekton Custom-Tasks Using Shipwright Builds #12
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: otaviof 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 |
openshift-ci
bot
added
the
approved
|
/assign @SaschaSchwarze0 @adambkaplan @coreydaley |
otaviof
force-pushed
the
BUILD-476
branch
3 times, most recently
from
a68dce1
to
8d3ef30
Compare
| return RequeueOnError(err) | ||
| } | ||
|
|
||
| var br = &v1alpha1.BuildRun{} |
There was a problem hiding this comment.
The logic here is missing code to handle a canceled Run. I suggest the following:
- If extra fields is empty (= BuildRun not yet created), then we do not create a BuildRun at all.
- If extra fields is not empty (= BuildRun was created), then we retrieve the BuildRun, check if it is not complete, if so, we cancel it (= set .spec.status to BuildRunCancel). This will need update permission to be added.
There was a problem hiding this comment.
Thanks for the suggestion! I added the logic to handle both Tekton Run and BuildRun getting cancelled reflecting the status on each other. Please consider the latest changes.
| Value: &v, | ||
| }) | ||
| } | ||
| } else { |
There was a problem hiding this comment.
Tekton has recently added ParamTypeObject. I guess we want to reject this.
There was a problem hiding this comment.
Do you have more examples about it? Thanks in advance.
There was a problem hiding this comment.
The Tekton TEP is https://github.com/tektoncd/community/blob/main/teps/0075-object-param-and-result-types.md.
The general purpose is to better group many related parameters into one.
We don't support them in Shipwright and I at the moment do not yet see the need for them anyway because a Build is just one step of a larger pipeline and does rarely have groups of parameters. Vs. in a larger Tekton pipeline you could have parameters related to source code retrieval (Git repo, Git branch), build related (context directory, Dockerfile if we stick with image builds as an example), and maybe also deployment related (kube namespace, etc.)
Your code should have an else if checking for the explicit type ParamTypeString and an else block with things we do not support. The whole if might then be rewritten as a switch but that's up to what you prefer.
otaviof
force-pushed
the
BUILD-476
branch
3 times, most recently
from
5b683f5
to
2c42230
Compare
otaviof
requested review from
SaschaSchwarze0
and removed request for
sbose78 and
adambkaplan
otaviof
force-pushed
the
BUILD-476
branch
3 times, most recently
from
1b8a910
to
adc25ba
Compare
| logger.V(0).Info("Marking the Tekton Run as successful!") | ||
| run.Status.MarkRunSucceeded(string(condition.Status), condition.Message) | ||
| } | ||
| run.Status.Conditions = append(run.Status.Conditions, condition) |
There was a problem hiding this comment.
This code is causing the list of conditions to grow while a BuildRun is running.
apiVersion: tekton.dev/v1alpha1
kind: Run
...
status:
conditions:
- lastTransitionTime: "2023-01-05T17:48:58Z"
status: Unknown
type: Succeeded
- lastTransitionTime: "2023-01-05T17:48:59Z"
message: Pending
reason: Pending
severity: Info
status: Unknown
type: Succeeded
- lastTransitionTime: "2023-01-05T17:49:12Z"
message: Not all Steps in the Task have finished executing
reason: Running
severity: Info
status: Unknown
type: Succeeded
extraFields:
buildRunName: shp-run-1672940937027-ko-build-mghj2
buildRunNamespace: build-system
startTime: "2023-01-05T17:48:58Z"
There was a problem hiding this comment.
Should it only have a single condition? I thought the conditions would be appended thus the last one represents the current state.
There was a problem hiding this comment.
For instance, checking pod for its conditions I found the following:
$ k get pods jenkins-412-7f44fdb94f-mp4t2 -o json |jq '.status.conditions'
[
{
"lastProbeTime": null,
"lastTransitionTime": "2023-01-05T10:42:03Z",
"status": "True",
"type": "Initialized"
},
{
"lastProbeTime": null,
"lastTransitionTime": "2023-01-06T04:31:15Z",
"status": "True",
"type": "Ready"
},
{
"lastProbeTime": null,
"lastTransitionTime": "2023-01-06T04:31:15Z",
"status": "True",
"type": "ContainersReady"
},
{
"lastProbeTime": null,
"lastTransitionTime": "2023-01-05T10:42:03Z",
"status": "True",
"type": "PodScheduled"
}
]
There was a problem hiding this comment.
An array of conditions can have multiple entries. But, per type, there should only be one.
There was a problem hiding this comment.
That makes sense. Let me amend it.
There was a problem hiding this comment.
Please consider the latest changes, I found out a knative helper to keep the conditions based on its type.
Watches over Tekton Run instances pointing to Shipwright Builds, and issues BuildRuns accordinly. Co-authored-by: Sascha Schwarze <schwarzs@de.ibm.com>
|
@SaschaSchwarze0 please consider. |
Including a few modifications to avoid leaking objects.
Changes
Tekton Custom-Tasks controller.
Submitter Checklist
Includes docs if changes are user-facingRelease Notes