-
Notifications
You must be signed in to change notification settings - Fork 19
Poll transmitter specified url #69
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
FragLegs
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you originally intended for this to be a documentation-only change. It might be easier if that's all it was and we left the renaming of method and endpoint_url for a different pull request.
| { | ||
| "delivery": { | ||
| "delivery_method": "urn:ietf:rfc:8935", | ||
| "method": "urn:ietf:rfc:8935", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked at the already approved spec and it appears that the spec uses delivery_method key.
"delivery": {
"delivery_method":
"https://schemas.openid.net/secevent/risc/delivery-method/push",
"url": "https://receiver.example.com/events"
},
https://openid.net/specs/openid-sse-framework-1_0-ID1.html#rfc.section.7.1.2.2
Though I would have liked the method over delivery_method (as described earlier), that would break the backwards compatibility of transmitter's APIs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The currently approved spec uses method in the spec (see sections 11.2.1.1. and 11.2.1.2), and mistakenly uses delivery_method in the examples.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for clarifying it :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@FragLegs @tulshi noticed discrepancy around endpoint_url vs url. The spec mentions endpoint_url (see sections 11.2.1.1. and 11.2.1.2)
And examples show url. Could we align on this either in this PR or a brand new?
timcappalli
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Closes #36 (previous commits incorrectly refer to #45)