Skip to content

Return 204 on /ack success instead of JSON true#2088

Merged
achamayou merged 10 commits into
microsoft:masterfrom
letmaik:letmaik/ack-204
Jan 18, 2021
Merged

Return 204 on /ack success instead of JSON true#2088
achamayou merged 10 commits into
microsoft:masterfrom
letmaik:letmaik/ack-204

Conversation

@letmaik
Copy link
Copy Markdown
Member

@letmaik letmaik commented Jan 18, 2021

Addresses first point of #1944.

@letmaik letmaik requested a review from a team as a code owner January 18, 2021 12:11
@achamayou
Copy link
Copy Markdown
Member

@letmaik thank you, that looks good. Since this is a REST API change, would you mind adding an entry to CHANGELOG.md?

We could use https://github.com/openapi-tools/open-api-diff, the markdown diff looks quite reasonable, and may even be generated automatically on release.

@letmaik
Copy link
Copy Markdown
Member Author

letmaik commented Jan 18, 2021

@letmaik thank you, that looks good. Since this is a REST API change, would you mind adding an entry to CHANGELOG.md?

Will do, I always forget.

I just realized that CCF only supports 200 when using set_auto_schema. Any suggestions on how to extend that?

@achamayou
Copy link
Copy Markdown
Member

Pinging @eddyashton, I think it's just a matter of adding a default argument to set_auto_schema() and using it line 337?

@eddyashton
Copy link
Copy Markdown
Member

eddyashton commented Jan 18, 2021

Yes - what @achamayou said should be sufficient.

@ghost
Copy link
Copy Markdown

ghost commented Jan 18, 2021

letmaik/ack-204@17695 aka 20210118.29 vs master ewma over 50 builds from 17034 to 17692
images

Comment thread tests/infra/member.py Outdated
letmaik and others added 2 commits January 18, 2021 14:36
Co-authored-by: Eddy Ashton <ashton.eddy@gmail.com>
@letmaik
Copy link
Copy Markdown
Member Author

letmaik commented Jan 18, 2021

@achamayou @eddyashton I had to do a bit more plumbing but I think I ended up at something good. If set_auto_schema is called with a void response schema then it defaults to 204 now.

@achamayou
Copy link
Copy Markdown
Member

@letmaik that does seem like a sensible default, but we may still want to support user-defined 2xx, like Created etc in the future. Agree it's out of scope for this change though.

@letmaik
Copy link
Copy Markdown
Member Author

letmaik commented Jan 18, 2021

@letmaik that does seem like a sensible default, but we may still want to support user-defined 2xx, like Created etc in the future. Agree it's out of scope for this change though.

This PR allows specifiying a custom status code. Isn't that what you mean?

@achamayou achamayou merged commit 35172b7 into microsoft:master Jan 18, 2021
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.

3 participants