-
Notifications
You must be signed in to change notification settings - Fork 584
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 CloudEvents extension for deprecated events #1307
Add CloudEvents extension for deprecated events #1307
Conversation
90dbdd7
to
af75d90
Compare
Introduced the `deprecation` attribute to indicate when an event type is deprecated, and added the `sunset` attribute to specify the date and time when the event will become unsupported. This extension provides clear guidelines and examples for implementing these attributes, aiming to improve lifecycle management and ensure better communication with consumers. Signed-off-by: Vandewilly Oliveira da Silva <[email protected]>
af75d90
to
7118488
Compare
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.
Definitely happy with the idea of being able to signal deprecation - definitely don't want to introduce more timestamp formats.
- Type: `String` | ||
- Description: Specifies the future date and time when the event type will | ||
become unsupported, formatted according to | ||
[RFC 8594](https://tools.ietf.org/html/rfc8594). |
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.
While I can see the purpose of using RFC 8594 here, CE isn't tightly bound to HTTP, and as it already has a Timestamp format. I'd definitely prefer this to be a Timestamp-typed attribute, whatever we call 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.
Changed type to Timestamp
|
||
- Type: `String` | ||
- Description: Indicates that the event type is deprecated. This can be a | ||
boolean `true` or a date in [RFC 5322](https://tools.ietf.org/html/rfc5322) |
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.
Rather than having a String that can be one of two formats, I'd prefer to have two attributes, each of which uses a single existing CE type - one Boolean
, one Timestamp
. (e.g. deprecated
and deprecatedfrom
)
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.
separated out the attributes:
- deprecation
- deprecationfrom
format indicating when the deprecation was announced. | ||
- Constraints | ||
- REQUIRED | ||
- If the value is a date, it MUST be formatted according to RFC 5322. |
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.
Why introduce a different (and relatively hard to parse, culture-sensitive) format when we already use RFC3339 for the CloudEvent Timestamp format?
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.
fixed
- A boolean deprecation: `"deprecation": "true"` | ||
- A date-based deprecation: `"deprecation": "Sun, 11 Aug 2024 23:59:59 GMT"` | ||
|
||
### sunset |
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.
If we do end up with two attributes above, I'd prefer this to be something starting with "deprecated" or "deprecation" - perhaps "deprecationsunset" is best, but it keeps them all together.
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.
Although a different purpose, the Deprecation
name is aligned with this IETF draft standard.
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.
Again, that's HTTP-centric though. While I don't mind if attribute names happen to coincide with HTTP ones, I don't think that should be a driving factor.
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.
Renamed to deprecationsunset
|
||
The `sunset` attribute SHOULD specify the exact date and time when the event | ||
will no longer be supported. This is the final cutoff date after which the | ||
event will no longer function as expected. |
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.
We should probably have some guidance around what different actors might do with an event that they see after this time. For example, if you're an "event forwarder" should you just drop such events?
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.
Added a new paragraph in the Usage
section. Please take a look at it.
needs to be in all CloudEvents, rather it needs to be included only when this | ||
extension is being used. | ||
|
||
## Attributes |
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'd like some sort of attribute describing a migration path - "stop using this, start using X". We could make that a URI instead of text, for people to link to docs. Not sure of the details, but something would be good.
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.
Added a new (optional) attribute: deprecationmigration
Signed-off-by: Vandewilly Oliveira da Silva <[email protected]>
23b9c41
to
2891960
Compare
Signed-off-by: Vandewilly Oliveira da Silva <[email protected]>
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.
This looks a lot better to me, thanks so much. Various additional questions...
- Constraints | ||
- REQUIRED | ||
- The value MUST be a boolean (`true` or `false`). | ||
- Example: `"deprecation": "true"` |
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'm torn between the consistency of having a common prefix, and the oddity of it sounding like it's not Boolean (whereas "deprecated: true" sounds fine).
Thinking about it more, when would this ever actually have a value of false? That would be unusual at least...
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.
Does this have to be REQUIRED?
If you just have a deprecated
: true and imply a default of false, there is no need to add a mandatory attribute.
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.
renamed to deprecated, and changed to OPTIONAL
officially marked as deprecated. | ||
- Constraints | ||
- OPTIONAL | ||
- If present, MUST adhere to the format specified in |
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 suspect we don't need this, because it's inherent in the type. (And only the string representation needs to adhere to a format anyway - if this is formatted with a Protobuf formatter for example, it'll end up as a protobuf timestamp.)
Ditto deprecationsunset
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.
we have that in the recordedtime extension: https://github.com/cloudevents/spec/blob/main/cloudevents/extensions/recordedtime.md?plain=1#L45-L46
I was just following the same convention
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'll happily create a PR to get rid of it from there :)
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.
Remove reference to RFC 3339
migrating to a new event or version. | ||
|
||
Consumers SHOULD use the `deprecation` and `deprecationfrom` attributes to | ||
monitor the lifecycle of events they rely on. If `deprecationsunset` has been |
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.
Presumably consumers should consider switching to the recommended replacement as soon as they see that the event is deprecated, really? (You don't want to wait until the sunset.) Perhaps this should be more "consumers should make efforts to switch to the recommended replacement before the specified sunset"?
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.
Rephrased this paragraph. Please take a look at it.
|
||
Consumers SHOULD use the `deprecation` and `deprecationfrom` attributes to | ||
monitor the lifecycle of events they rely on. If `deprecationsunset` has been | ||
reached, consumers SHOULD consider switching to the RECOMMENDED replacement, as |
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 don't think RECOMMENDED needs to be in capitals here.
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.
there is a lint script that complains about that. I'll reword it to suggested
.
- Type: `Timestamp` | ||
- Description: Specifies the future date and time when the event type will | ||
become unsupported. | ||
- Constraints |
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 wonder whether it's worth constraining this to say that the sunset for a type should never be observed to become earlier, but may be observed to become later. (An event producer may choose to support an event for longer than initially expected, but shouldn't shorten that support window.)
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.
added a constraint.
- Type: `Timestamp` | ||
- Description: Specifies the date and time when the event type was | ||
officially marked as deprecated. | ||
- Constraints |
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.
Should we have a constraint around the stability of this? (It would be odd for it to vary over time.)
Should this always be in the (potentially very near) past or does it make sense to pre-announce deprecation?
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.
added a new constraint.
@vandewillysilva : Sorry, I didn't have time to look deeper into it, but just wanted to drop some links: Here is a RFC for sunset as a header: https://datatracker.ietf.org/doc/html/rfc8594 At my company, SAP we have also standardized the same attributes as you propose here. In case you want to have a look, they're documented in the Open Resource Discovery specification, here: https://sap.github.io/open-resource-discovery/spec-v1/interfaces/document#event-resource_releasestatus In particular, check Also, we noticed that deprecating a resource does not necessarily have to imply a sunset. Of course you have to deprecate first, and sometimes you already plan the sunset when you do. But sometimes, you just created a successor resource and want just to indicate that the old resource is deprecated and the newer should be used. |
Signed-off-by: Vandewilly Oliveira da Silva <[email protected]>
438f98c
to
0a9af27
Compare
Signed-off-by: Vandewilly Oliveira da Silva <[email protected]>
@Fannon thanks for the links and insights. It's great to see that we're aligned on similar attributes. :) |
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.
One more nit - but basically I'm otherwise happy :)
`false` if not specified. | ||
- Constraints | ||
- OPTIONAL | ||
- The value MUST be a boolean (`true` or `false`). |
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.
Just like the timestamp part, I don't think we need to say this - it's implicit in the type being Boolean.
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.
agreed. Removed the constraint
Signed-off-by: Vandewilly Oliveira da Silva <[email protected]>
A must have feature |
@duglin Do you think it’s a good idea to proceed with this deprecation extension proposal? Let me know if there’s any other work needed or if you have any additional feedback. |
`false` if not specified. | ||
- Constraints | ||
- OPTIONAL | ||
- Example: `"deprecated": "true"` |
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.
"deprecated": true
no quotes
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.
fixed
officially marked as deprecated. | ||
- Constraints | ||
- OPTIONAL | ||
- The `deprecationfrom` timestamp SHOULD remain stable once set and SHOULD |
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.
Just curious, aside from this being a possible "FYI" bit of info, do people use this timestamp for some purpose in real life? Doesn't seem like it would influence people's action.... the "sunset" one is the important one.
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 deprecationsunset
dictates the hard deadline, but deprecationfrom
can prompt consumers to start planning their migration earlier.
IMO, it adds valuable context and encourages proactive behavior from consumers.
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.
In theory, but I would think the presence of any of these would indicate that the event is deprecated regardless of when the deprecation period stated, right?
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.
that's correct.
deprecationfrom
adds an extra layer of clarity. This can be useful for teams who need to plan their migrations earlier rather than waiting until the sunset approaches.
consumers transition away from the deprecated event. | ||
- Constraints | ||
- OPTIONAL | ||
- The URI SHOULD point to valid and accessible resources that help the |
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.
s/help/helps/
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.
fixed
Signed-off-by: Vandewilly Oliveira da Silva <[email protected]>
|
||
## Attributes | ||
|
||
### deprecated |
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.
Since we basically say that even REQUIRED fields are really optional if the extension isn't used, I'm wondering if this one should be REQUIRED. It would be kind of weird to have this one "false" but any of the other attributes present, right?
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.
That's a valid point. The initial proposal had this attribute as required:
#1307 (comment)
but I believe that requiring this field will help to ensure clarity and prevent contradictory states. Will make the changes.
Yup - it's on the agenda for tomorrow's call. Will you be on the call? Not a requirement but it would be nice :-) |
Signed-off-by: Vandewilly Oliveira da Silva <[email protected]>
I have a conflict, but I'll try to join. I can't guarantee I'll make it. |
PR is failing due to broken links in avro spec. This PR #1309 fixes the issue. |
Signed-off-by: Vandewilly Oliveira da Silva <[email protected]>
@duglin I went over today's recording and made the changes related to the deprecated field. Please take a look at it. |
Signed-off-by: Vandewilly Oliveira da Silva <[email protected]>
- Type: `Boolean` | ||
- Description: Indicates whether the event type is deprecated. | ||
- Constraints | ||
- MUST be true |
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.
s/true/`true`/
@vandewillysilva just one minor tweak and then we can merge |
Signed-off-by: Vandewilly Oliveira da Silva <[email protected]>
Good catch! Fixed. |
Approved on the 8/22 call. |
Introduced the
deprecation
attribute to indicate when an event type is deprecated, and added thesunset
attribute to specify the date and time when the event will become unsupported. This extension provides clear guidelines and examples for implementing these attributes, aiming to improve lifecycle management and ensure better communication with consumers.