ietf-wg-httpapi / deprecation-header

Repository for IETF WG draft deprecation-header
Other
5 stars 5 forks source link

Feedback from AD Francesca Palombini #30

Closed sdatspun2 closed 1 week ago

sdatspun2 commented 2 months ago

On Sun, Jul 7, 2024 at 5:53 AM Francesca Palombini francesca.palombini@xxx wrote:

AD Review of draft-ietf-httpapi-deprecation-header-04

cc @fpalombini

Thank you for the work on this document.

I have a couple of comments and questions. Nothing major, but I'd like to see a new revision before starting the IETF Last Call.

Francesca

Comments

Abstract

(in the sense of [URI])

Please remove the square brackets, which indicate a reference (which is not included in the draft at the moment). The link to STD 66 is missing - I believe Mark meant that it should be added, in his comment. However as the abstract should stand on its own, there is no need to add that reference here. If you do want to add it, it should be added to the introduction.

Documentation

Section 3.1:

This may be documentation explaining the use of the

Deprecation header field, and also explaining under which

circumstances and with which policies (announcement before

deprecation; continued operation after deprecation) deprecation might

be happening.

I am uncertain about the "explaining the use of the Deprecation header field", what does that mean? I would assume the use is self explanatory. Maybe some additional text would help. In general, an example of this documentation would have been useful.

SHOULD keep functioning

Section 5:

Deprecated resources SHOULD keep functioning as before, allowing

consumers to still use the resources in the same way as they did

before the resources were declared deprecated.

This SHOULD will for sure come up during IESG evaluation, if we don't address it now. If SHOULD is used, then it must be accompanied by at least one of: (1) A general description of the character of the exceptions and/or in what areas exceptions are likely to arise. Examples are fine but, except in plausible and rare cases, not enumerated lists. (2) A statement about what should be done, or what the

considerations are, if the "SHOULD" requirement is not met. (3) A statement about why it is not a MUST.

Section 7

I am not sure what this section adds, as it contains the same example that can be found elsewhere in the text.

Security considerations

Section 7.1:

I am sure this was a copy paste gone wrong, and the section should not be a sub-section of Section 7.

SHOULDs in Sec cons

Same comment as above about the SHOULDs in Section 7.1.

The Deprecation header field SHOULD be treated as a hint, meaning

that the resource is indicating (and not guaranteeing with certainty)

that it will be or is deprecated.

This doesn't need to be BCP14 SHOULD, IMO.

Applications consuming the

resource SHOULD check the resource documentation to verify

authenticity and accuracy.

What would be the consequence on not following this recommendation, or what exceptions make this a SHOULD rather than a MUST?

Applications consuming the resource SHOULD

check the referred resource documentation to verify authenticity and

accuracy.

This seems duplicated text from the first paragraph.

References:

** Obsolete normative reference: RFC 7234 (Obsoleted by RFC 9111) - please fix.

** Please fix the SFBIS reference: see [SCRUCTURED-FIELDS] in https://datatracker.ietf.org/doc/html/draft-ietf-httpapi-link-template-04#name-normative-references for the correct way to reference a draft.

Nits

Section 3.1

However, the resource already exposes a link where

information is available how deprecation is managed for the resource

context.

Maybe s/is available how deprecation/is available on how deprecation/

Notes

This review is in the "IETF Comments" Markdown format, You can use the

ietf-comments tool to automatically convert this review into

individual GitHub issues.

sdatspun2 commented 1 month ago

Hi Francesca,

Thanks for your comments. Indeed, you caught a copy-paste gone wrong. I was moving sections around and messed up. I think I have addressed most of the comments except one about SHOULD keep functioning. I am not sure how to provide an example of documentation regarding deprecation. The example that comes to mind is something like Square's API, e.g. https://developer.squareup.com/reference/square/customers-api/delete-customer-card that shows how deprecation applies. However, I am not sure how in an IETF draft we can accommodate such. Imo, it is out of scope of this draft to show how to document deprecation of a resource. @dret, correct me if I am wrong.

dret commented 1 month ago

I am uncertain about the "explaining the use of the Deprecation header field", what does that mean? I would assume the use is self explanatory. Maybe some additional text would help. In general, an example of this documentation would have been useful.

In my mind it's about policy. For example you might say something like "Deprecation will always be signaled in the dedicated places at least X days ahead of the planned deprecation date." Or you might deprecate things and only signal deprecation afterwards. As a consumer of an API it would be useful to know this kind of policy.

sdatspun2 commented 1 month ago

Pl. review Security Considerations again. Here is commit https://github.com/ietf-wg-httpapi/deprecation-header/commit/0e21fdf9590d5820ed37917d9929e3c92d2b4016

richsalz commented 1 month ago

Made a couple of minor suggestions.

sdatspun2 commented 1 month ago

Thanks @richsalz. Please take a look at the latest.

richsalz commented 1 month ago

Looks good to me! Thanks.

fpalombini commented 1 month ago

Thank you! I reviewed the changes and left a couple of comments.

Additionally to that, I think 2 comments were not addressed.

  1. "SHOULD keep functioning as before" - I think it still misses context text about why is this not a MUST.
  2. "explaining the use of the Deprecation header field" - @dret answers exactly what I was looking for, not necessarily a specific documentation example, more of a "this is the sort of thing, and this is what we mean by "explaining the use"". Maybe rephrasing his response and adding it as context in the doc would be useful (it would have helped me).
sdatspun2 commented 1 month ago

Thanks. I will let @dret commit the change, perhaps it was in works at his end.

dret commented 1 month ago

i am probably missing some context here but here is my take:

On 2024-07-23 23:11, fpalombini wrote:

  1. "SHOULD keep functioning as before" - I think it still misses context text about why is this not a MUST.

sometimes providers slow down deprecated services as an encouragement for people to migrate, or they may do other things to encourage migration. i guess technically you could say that "functionally they MUST" and "non-functionally they MAY NOT", but i have no idea how this kind of consideration is best phrased in IETF terminology.

  1. "explaining the use of the Deprecation header field" - @dret https://github.com/dret answers exactly what I was looking for, not necessarily a specific documentation example, more of a "this is the sort of thing, and this is what we mean by "explaining the use"". Maybe rephrasing his response and adding it as context in the doc would be useful (it would have helped me).

that's good to hear. in the end deprecation mostly is a policy issue, not something strictly functional. so making that policy explicit is a good thing.

sdatspun2 commented 1 month ago

@dret Pl. commit changes re policy sentence. Thanks.

fpalombini commented 2 weeks ago

Hi! Any updates? If we don't have text to add, I am ok with just sending the next version with the current implemented changes through (see my comment as an improvement request, but non-blocking). This might come up again in Last Call or IESG eval, but we can address it then.

sdatapix commented 2 weeks ago

@fpalombini Most of the changes except one are incorporated in the latest draft. I am waiting on @dret to look at the draft and change what he was suggesting. However, if you are ok, we can publish a new draft and send it forward. @sdatspun2

dret commented 2 weeks ago

On 2024-08-21 00:36, fpalombini wrote:

Hi! Any updates? If we don't have text to add, I am ok with just sending the next version with the current implemented changes through (see my comment as an improvement request, but non-blocking). This might come up again in Last Call or IESG eval, but we can address it then.

In https://github.com/ietf-wg-httpapi/deprecation-header/issues/30#issuecomment-2246325501 you indicated that the policy language I suggested would be fine to address your comment. @sdatspun2 would be the one knowing whether that suggestion made it into the draft or not. If it is in there, we should be good to publish a new version.

sdatapix commented 2 weeks ago

@fpalombini @dret pl review https://github.com/ietf-wg-httpapi/deprecation-header/commit/25738aaa4d8bb7ecc9d7a2e04ff7250f06a1969a.

sdatspun2 commented 1 week ago

Draft #5 published, addresses comments.