-
Notifications
You must be signed in to change notification settings - Fork 16
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
feature/sunset #102
feature/sunset #102
Conversation
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 really like this feature.
|
||
## Objectives | ||
|
||
Let application operation teams know that a given operation depends on an API which is slated for removal so they can proactively update the application before a breakage occurs. |
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.
For more context, would adding that this handler uses the Sunset header only to communicate that a resource is slated for removal. In the RFC, a resource might communicate about sunset policy by using the sunset link but that is not handled in the scope of this handler.
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 those are mutually exclusive. From reading the RFC my understanding is that the link can be provided even in a deprecation scenario.
And there's nothing in the RFC that'd allow us to mechanically parse the sunset "type" either. So from a client middleware perspective, I think we need to treat them as equivalent.
Now, you bring a good point, maybe we should also parse the link header and add it to the event attributes when present so admins have a hint to where start looking at. I'll make that addition.
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.
(sorry apparently the pr was on auto-merge and when I pushed the resolution of the conflict, it got merged)
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.
Parsing the link and adding it as an event is a good idea. So, either header or link.
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
fixes #89