Conversation
Update Github Actions to use actions/checkout v4 (previously v3). Signed-off-by: Rahul Gupta <CxRes@users.noreply.github.com>
Adds a new case for "Container only" triggers for PUT/PATCH methods being used to create a resource inside a container resulting in a 201 response. Signed-off-by: Rahul Gupta <CxRes@users.noreply.github.com>
PUT and PATCH requests successfully modifying a resource may result in a 205 response and therefore SHOULD be notification triggers (See #1). Signed-off-by: Rahul Gupta <CxRes@users.noreply.github.com>
Status code are now ordered numerically for the POST method in Resource only triggers. Signed-off-by: Rahul Gupta <CxRes@users.noreply.github.com>
(unless creating child) Unless POST is being used to create a child resource, it should also trigger the resource's parent container to send notifications. Signed-off-by: Rahul Gupta <CxRes@users.noreply.github.com>
joachimvh
reviewed
Oct 27, 2023
joachimvh
left a comment
joachimvh
left a comment
There was a problem hiding this comment.
I personally feel that the differentiation in all the different sections makes it more complicated to understand when notifications should be sent out.
Why not have 1 trigger section that states that if you do a PUT/PATCH/POST/DELETE and receive a 200/201/204/205 it can trigger a notification? The section below already details how creating or updating a resource can trigger specific notifications on the resource or its container.
Member
Author
|
I want to be explicit about from where notifications are being sent out in each specific scenario (rather than have implementors work that out, which might lead to subtle variations between implementors). What I might propose in the spirit of your suggestion is to have one table with a third column saying "Notify on" (we could bikeshed that) with values of "resource", "container" and "resource and container". Would that be more readable? |
Member
Author
|
All this gives me an idea of how to rewrite the trigger section when I include non-HTTP triggers! So, I am thinking to make the unified table, merge this PR, and then I will seek your opinion after the rewrite on the whole section. |
Add a missing indefinite article to "PREP notification". Co-authored-by: Joachim Van Herwegen <joachimvh@gmail.com>
Based on the discussions in PR #3, triggers have been combined into a single table for easier comprehension. Signed-off-by: Rahul Gupta <CxRes@users.noreply.github.com>
CxRes
added a commit
that referenced
this pull request
Oct 30, 2023
Based on discussions in #1 and #3, the Triggers section has been completely re-written: + Normative requirements only specify notifications being triggered whenever the state of a LDP Resource is modified. + HTTP specific triggers are moved to implementation guidance: + PUT/PATCH requests being used to create a resource inside a container resulting in a 201 response are notification triggers. + PUT/PATCH requests successfully modifying a resource with 205 response are notification triggers. + Unless POST is being used to create a child resource, it should also trigger the resource's parent container to send notifications. + HTTP specific triggers have been combined into a single table for easier comprehension. Signed-off-by: Rahul Gupta <CxRes@users.noreply.github.com>
Merged
CxRes
added a commit
that referenced
this pull request
Oct 31, 2023
Based on discussions in #1 and #3, the Triggers section has been completely re-written: + Normative requirements only specify notifications being triggered whenever the state of a LDP Resource is modified. + HTTP specific triggers are moved to implementation guidance: + PUT/PATCH requests being used to create a resource inside a container resulting in a 201 response are notification triggers. + PUT/PATCH requests successfully modifying a resource with 205 response are notification triggers. + Unless POST is being used to create a child resource, it should also trigger the resource's parent container to send notifications. + HTTP specific triggers have been combined into a single table for easier comprehension.
CxRes
added a commit
that referenced
this pull request
Oct 31, 2023
Based on discussions in #1 and #3, the Triggers section has been completely re-written: + Normative requirements only specify notifications being triggered whenever the state of a LDP Resource is modified. + HTTP specific triggers are moved to implementation guidance: + PUT/PATCH requests being used to create a resource inside a container resulting in a 201 response are notification triggers. + PUT/PATCH requests successfully modifying a resource with 205 response are notification triggers. + Unless POST is being used to create a child resource, it should also trigger the resource's parent container to send notifications. + HTTP specific triggers have been combined into a single table for easier comprehension.
Member
Author
|
Abandoned in favour of #4. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR adds PUT and PATCH methods with 201 and 205 responses as notification triggers.
External (Non-HTTP method) triggers will be dealt separately.