Remove the Trigger field from TaskRun/PipelineRun. #857
Conversation
googlebot
added
the
cla: yes
tekton-robot
added
the
size/L
|
[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
|
/retest |
dlorenc
force-pushed
the
notrigger
branch
2 times, most recently
from
1c0d126
to
56f7fc6
Compare
|
|
|
@dlorenc: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
Hmm... it's not completely unused in that the PipelineRun controller sets the field on TaskRuns, and I think @iancoffey was saying that the event implementation sets it. I don't mind removing it, but it seems odd to remove it now before we decide how to replace it. Maybe bring it up at the WG meeting this week @dlorenc ? |
This field is not currently used, and it's usage should be replaced with labels/annotations. This commit removes it now, before anyone takes a dependency on it. At a later time we can decide on the correct label/annotation usage for this information and start writing that data.
dlorenc
changed the title
Fixed! |
If we know we want to remove it, I'd rather remove it ASAP before other things start using it. I added it to the agenda for tomorrow. |
|
Indeed, I had thought to implement this for the eventbinding things, but am fine just using labels instead 👍 |
|
/hold LGTMing but also putting on hold until after WG tomorrow. =) |
tekton-robot
added
do-not-merge/hold
|
/hold |
|
ah, it added the label but I didn't see it. |
I would also argue that merely writing a field doesn't count as using it :) |
Yeah that's a good point - I guess it only matters if someone is consuming it! |
|
I think an annotation would be more fitting, the spec should contain information for the controller to reconciliate the resource, while "trigger" is not used/needed by the controller at all, so I'm +1 on removing it as a field. |
|
Discussed it (real quick) during the working group, as nobody raise any flag for keeping it, let's get rid of the field 👼 |
tekton-robot
removed
the
do-not-merge/hold
The trigger field was removed in tektoncd/pipeline#857 and should have been removed from the dashboard code in the intervening time.
The trigger field was removed in tektoncd/pipeline#857 and should have been removed from the dashboard code in the intervening time.
With CEL interceptor writing to the `extensions` field, it is impossible to chain added fields from a CEL interceptor to a webhook interceptor today. This is because we do not pass the extensions field to the webhook interceptor yet. This commit attempts to fix this by merging any extensions to the body before sending it over to the Webhook interceptor. This is temporary until we fully move webhook interceptors to the new pluggable interface. One implication of this change is that the body may now contain an `extensions` field separate from the top level extensions field if one uses the Webhook interceptor. In addition, the CEL environment also did not have access to extensions which I fixed. Fixes tektoncd#857 Signed-off-by: Dibyo Mukherjee <dibyo@google.com> Add extensions support to CEL
Changes
This field is not currently used, and it's usage should be replaced
with labels/annotations. This commit removes it now, before anyone
takes a dependency on it.
At a later time we can decide on the correct label/annotation usage
for this information and start writing that data.
Ref #833
Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
Release Notes