Update status code handling #1077
Conversation
tekton-robot
added
the
release-note
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
tekton-robot
added
the
size/L
|
The following is the coverage report on the affected files.
|
|
considering the amount of deleted code...that's not terrible for a delta of coverage. i'll see what is missing now. |
jmcshane
force-pushed
the
feature/update-status-code-handling
branch
from
424db8b to
0d9d86e
Compare
|
The following is the coverage report on the affected files.
|
jmcshane
force-pushed
the
feature/update-status-code-handling
branch
from
0d9d86e to
e062e07
Compare
|
The following is the coverage report on the affected files.
|
In the current version of the product, the HTTP request to the EventListener endpoint waits for all triggers to process to determine if a resource was created. This is not in line with the suggestion by the source control systems for webhook endpoints. This change updates the endpoint to respond as soon as the triggers have been selected for processing.
jmcshane
force-pushed
the
feature/update-status-code-handling
branch
from
e062e07 to
490b897
Compare
|
The following is the coverage report on the affected files.
|
|
/test pull-tekton-triggers-integration-tests |
|
Since this is a breaking change, should we announce it and give users more of a heads up before releasing it? |
Yeah, im not sure how those announcements are normally done. We can discuss in WG next week, or potentially API working group to talk about how these types of behavior updates should be managed between the maintainers and the end user community. We saw a similar thing happen with the install for interceptors for v0.13 and there were a few people that popped in the Slack to discuss the change. I think the best course of action will be to have a note about the change in behavior in the release notes and potentially have some more thorough documentation in the eventlistener doc about status code behavior. |
So, the channels we have are
Yeah I expect some questions on Slack despite announcements. In hindsight, I should have announced the interceptors release yaml changes more widely. |
|
@jmcshane: PR needs rebase. 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. |
tekton-robot
added
the
needs-rebase
| localRequest := request.Clone(request.Context()) | ||
| if err := r.processTrigger(t, localRequest, event, eventID, eventLog); err != nil { |
There was a problem hiding this comment.
So yeah it still fails. The user presumably learns of it from some other means. Full disclosure, I did not research how that might now be or not be achieved.
If there is an easy enough reader's digest explanation @jmcshane @dibyom please elaborate. Or if a pointer to what I have to read to understand makes more sense, that works too.
thanks
There was a problem hiding this comment.
I think the diff for the docs/eventlisteners.md page shows the change: https://github.com/tektoncd/triggers/pull/1077/files#diff-7da54b4f5484ec784351290e106d6aebea551a17046b6116cc8e65964ebb693f
i.e instead of waiting for a resource to be created and then responding with a 201 status code, Triggers will now return a 202 response earlier (i.e. when it has selected the triggers to process -- it will no longer wait for the trigger processing to complete)
There was a problem hiding this comment.
OK so yeah that markdown, assuming basic HTTP rc knowledge, makes it clear that the processing here is now asynchronous vs. synchronous.
With all the scalability advantages, etc. obtained with that shift.
In theory then the poster of the event then needs to know what was triggered by the event and then query those artifacts directly for status.
Quick code scan / memory refresh and I see the /triggers-eventid label set on the objects created (which we have had for a while) facilitates that query.
Would a cross reference to https://github.com/tektoncd/triggers/blob/4b9aa3157f3989255f350c8c60c2b6aecab38783/docs/triggertemplates.md#structure-of-a-triggertemplate somewhere and its explanation of the label near https://github.com/tektoncd/triggers/pull/1077/files#diff-7da54b4f5484ec784351290e106d6aebea551a17046b6116cc8e65964ebb693f be a useful change in your opinion @dibyom ?
Other than that potential tweak, I'm good. Thanks.
There was a problem hiding this comment.
Ok yeah, I think adding a couple of lines or (maybe link to a different section) with the information on querying makes sense.
|
Closing -- This commit has been migrated to #1132 |
|
/close |
|
@dibyom: Closed this PR. In response to this:
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. |
Changes
Closes #931
Removes status code handling in HTTP requests for triggers. Replacing with waitgroup that counts current number of triggers that are being processed by the Sink.
Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
See the contribution guide for more details.
Release Notes