-
Notifications
You must be signed in to change notification settings - Fork 860
Warn on server warning 'X-HF-Warning' #3589
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
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
hanouticelina
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.
Looks good, thanks! I left one comment about whether we should show all topic-less warnings or not
julien-c
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.
maybe use this PR to give a bit of context to the moon-landing team btw (in a moon-landing PR?) as i haven't yet shared a lot of context:)
LysandreJik
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.
Looks good to me, nice and robust test
I've created an issue -internal link- in moon with explanations (didn't want to start a PR just to give some details^^) |
thanks! |
This PR adds a helper to emit warnings on server request. This can be useful when a request completed successfully but we still want to warn the user about something. In practice we are checking the
X-HF-Warningresponse header whenhf_raise_for_statusis called.Expected format:
To avoid spamming the user, only 1 warning per topic is printed to the user. The topic itself is not printed, it is just a way to deduplicate them. Topic deduplication is case-insensitive. Topic is optional, in which case only the first non-labelled warning is printed.
A single HTTP request can contain several warnings by setting several times the
X-HF-Warning(TIL headers are not unique^^).Other alternatives explored:
Warningheader: long-deprecated and not used in practiceContent-Warningdraft: only a draft -so not yet a standard- + too much complexity for our use case. In this draft proposal, the warning message is set in the JSON body which is not as flexible as our approach. Warnings in body is theoretically more robust and powerful -with no size limit- but for our use case, using the header is fine (+ there is no body in HEAD calls + body is not always a JSON).