Skip to content
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

Add Stream ID #9

Merged
merged 16 commits into from
Jan 4, 2023
Merged

Add Stream ID #9

merged 16 commits into from
Jan 4, 2023

Conversation

FragLegs
Copy link
Contributor

@FragLegs FragLegs commented May 4, 2022

This PR does a number of things:

  • Most notably, it implements the changed described in issue Adding a Stream ID #4 - Adding a Stream ID
  • It reorders the sections so that information about creating/updating/deleting the stream comes before information about setting/getting the stream status
  • It adds Shayne Miel as a coauthor
  • It adds some info to the README to help others who might work on the spec

@FragLegs
Copy link
Contributor Author

FragLegs commented May 4, 2022

However, there is a slight wrinkle with the stream ID that I didn't foresee. How should we indicate to transmitters and receivers that they need a way to identify which stream is being POLLed or PUSHed?

For PUSH it is easy - we can leave it up to the Receiver to set either a URL param in the endpoint or use a specific endpoint that dictates the stream ID. It is a little complicated though since the Receiver does not know the stream ID when creating the stream, which would force them to provide the "delivery" info during a second request. How much of this should we call out in the spec?

For POLL it is up to the Transmitter to do the right thing and add a URL param or a specific endpoint for each stream. Do we need to call that out in the spec?

A third option would be to extend the POLL spec to include a stream_id parameter, but that seems like a big lift.

@adeinega
Copy link
Contributor

adeinega commented Aug 9, 2022

As an (almost) completely random comment, it would be great to keep only *.md documents in the repo, for example, openid-sse-framework-1_0.md, and then

  1. use https://github.com/cabo/kramdown-rfc to convert *.md to *.xml
  2. convert *.xml using xml2rfc to txt and HTML files when you they need to be published in other (external) places

It'll allow keeping the number of changes very low and easily review them. Moreover, Markdown files are convenient to edit even though the Web UI of GitHub.

@timcappalli
Copy link
Member

As an (almost) completely random comment, it would be great to keep only *.md documents in the repo, for example, openid-sse-framework-1_0.md, and then

  1. use https://github.com/cabo/kramdown-rfc to convert *.md to *.xml
  2. convert *.xml using xml2rfc to txt and HTML files when you they need to be published in other (external) places

It'll allow keeping the number of changes very low and easily review them. Moreover, Markdown files are convenient to edit even though the Web UI of GitHub.

that's the plan. Just haven't had a change to convert the others to MD and then set up GH actions

@FragLegs
Copy link
Contributor Author

However, there is a slight wrinkle with the stream ID that I didn't foresee. How should we indicate to transmitters and receivers that they need a way to identify which stream is being POLLed or PUSHed?

For PUSH it is easy - we can leave it up to the Receiver to set either a URL param in the endpoint or use a specific endpoint that dictates the stream ID. It is a little complicated though since the Receiver does not know the stream ID when creating the stream, which would force them to provide the "delivery" info during a second request. How much of this should we call out in the spec?

For POLL it is up to the Transmitter to do the right thing and add a URL param or a specific endpoint for each stream. Do we need to call that out in the spec?

A third option would be to extend the POLL spec to include a stream_id parameter, but that seems like a big lift.

I added notes in the PUSH/POLL section to indicate the uniqueness needs of the URLs.

@tulshi
Copy link
Contributor

tulshi commented Sep 15, 2022 via email

@FragLegs
Copy link
Contributor Author

FragLegs commented Sep 16, 2022 via email

update the Event Stream configuration and status, to add and remove
subjects and to trigger verification.
Transmitters which can be used by Event Receivers to create and
delete one or more Event Streams, query and update the configuration
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we are talking about multiple event streams now, we should probably write a new section that defines what an event stream is.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added

openid-sse-framework-1_0.txt Outdated Show resolved Hide resolved
openid-sse-framework-1_0.txt Outdated Show resolved Hide resolved
would make the following request to the Issuer
"https://tr.example.com/issuer1" to obtain its Configuration
information, since the Issuer contains a path component:
If the Issuer value contains a path component, any terminating / MUST
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've noticed that the quotes are gone from many places in the spec. Is that intentional or an artifact of the tools you are using?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will be fixed by #35

path component, if any. The syntax and semantics of ".well-known"
are defined in [RFC5785]. "sse-configuration" MUST point to a JSON
at the path formed by inserting the string /.well-known/sse-
configuration into the Issuer between the host component and the path
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the quotes are important here, otherwise the spec cannot be read unambiguously

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will be fixed by #35

openid-sse-framework-1_0.txt Show resolved Hide resolved

Table 6: Read Stream Status Errors

Examples:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These examples apply to all API calls. Should we move them to a part of the API description that applies to all calls?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. I think the examples sections all need a bit of rework. Some endpoints don't have examples, others (like this one) have examples that are too general. Since this was already in the spec, I'd like to leave it for a different pull request that is focused strictly on examples, if that's alright.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #37


7.1.2.2. Updating a Stream's Status

An Event Receiver updates the current status of a stream by making an
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should mark this for a later pull request, but this needs to be changed to a PATCH since it's an update.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 I'll make an issue for this and the examples comment above.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #38

openid-sse-framework-1_0.txt Outdated Show resolved Hide resolved
openid-sse-framework-1_0.txt Outdated Show resolved Hide resolved
openid-sse-framework June 2021

subject
OPTIONAL. Specifies the Subject Principal for whom the status has
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is optional, then we should specify that omitting the subject from the event means that the event applies to all subjects in the stream.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

Copy link

@topherMarie topherMarie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I flagged a few typographical issues. As I am just now familiarizing myself with the spec, I'm not sure my approval should count for much.

stream_id
*Read-Only*, A string that uniquely identifies the stream. Stream
IDs MAY be reused across Receivers, but MUST be unique per
Reciever. This value is generated by the Transmitter when the

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/Reciever/Receiver

Subject Principals are the managed entities in a SSE Transmitter or
Receiver. These include human or robotic principals, devices,
customer tenants in a multi-tenanted service, organizational units
within a tenant, groups of subject principals or other entities that

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally like the Oxford comma for lists like this.

groups of service principals, or other entities...

error code. See Security Considerations (Section 9).
empty 200 OK response. The Event Transmitter MAY choose to silently
ignore the request, for example if the subject has previously
indicated to the transmitter that they do not want events to be

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Transmitter is typically capitalized in this doc.

Copy link
Member

@timcappalli timcappalli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor stuff relating to errors and status codes

openid-sse-framework-1_0.xml Outdated Show resolved Hide resolved
openid-sse-framework-1_0.xml Show resolved Hide resolved
openid-sse-framework-1_0.xml Show resolved Hide resolved
openid-sse-framework-1_0.xml Outdated Show resolved Hide resolved
openid-sse-framework-1_0.xml Outdated Show resolved Hide resolved
openid-sse-framework-1_0.xml Show resolved Hide resolved
openid-sse-framework-1_0.xml Show resolved Hide resolved
openid-sse-framework-1_0.xml Show resolved Hide resolved
openid-sse-framework-1_0.xml Show resolved Hide resolved
openid-sse-framework-1_0.xml Show resolved Hide resolved
Copy link
Contributor

@tulshi tulshi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor comments, but LGTM otherwise.

openid-sse-framework-1_0.xml Outdated Show resolved Hide resolved
Copy link
Member

@timcappalli timcappalli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@FragLegs FragLegs merged commit d8b302c into main Jan 4, 2023
@FragLegs FragLegs deleted the stream_id branch January 4, 2023 19:52
@FragLegs FragLegs mentioned this pull request Jan 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants