Skip to content
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

[Personalizer] Rest API review #15696

Closed
slahabar opened this issue Aug 18, 2021 · 2 comments · Fixed by #15699
Closed

[Personalizer] Rest API review #15696

slahabar opened this issue Aug 18, 2021 · 2 comments · Fixed by #15699
Assignees
Labels
APIStewardshipBoard-SignedOff The Azure API Stewardship team has reviewed and approved the changes.

Comments

@slahabar
Copy link

slahabar commented Aug 18, 2021

Swagger PR: #15699

Design link
https://microsoft.sharepoint.com/:w:/t/personalizr/Ebn16DbDmadPuTMggz8FrzIBAsjvBQ4q1X41xsqVMCzkqA?e=qFDOsV

@ghost ghost added the needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. label Aug 18, 2021
@czubair czubair added the APIStewardshipBoard-ReviewRequested This should be reviewed by the Azure API Stewardship team in partnership with the service team. label Aug 18, 2021
@ghost ghost removed the needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. label Aug 18, 2021
@slahabar slahabar changed the title [Personalizer] Rest API review [Personalizer] Rest API review https://microsoft.sharepoint.com/:w:/t/personalizr/Ebn16DbDmadPuTMggz8FrzIBAsjvBQ4q1X41xsqVMCzkqA?e=qFDOsV Aug 18, 2021
@slahabar slahabar changed the title [Personalizer] Rest API review https://microsoft.sharepoint.com/:w:/t/personalizr/Ebn16DbDmadPuTMggz8FrzIBAsjvBQ4q1X41xsqVMCzkqA?e=qFDOsV [Personalizer] Rest API review Aug 18, 2021
@mikekistler
Copy link
Member

mikekistler commented Aug 19, 2021

Attending:
@slahabar
@mikekistler
@johanste
@tg-msft

The version should probably be v1.1-preview.2.

JS: Are the authentication protocols all the same here? Yes.

JS: What is the format of the request bodies? Just binary, or is there any specification? There is a definition in the repo.

TG: How is versioning handled for the payload? It is in the payload.

JS: There should be client libraries for C#, Java, JavaScript, Python. This is a PLR requirement. You should be planning for that.

Main customer for these APIs is first party, but will be making available to third parties after.

SL: Should we combine these two operations into one?
Keeping separate endpoints seems better from a resource-oriented perspective.
But thinking of these payloads as messages that are just getting posted to EH ... that's not the case tho.

TG: You should have a default response that describes the error response.
Should be the same error response for all operations.

MK: If no response body, then return 204 rather than 200.

Swashbuckle generates the body as "type: string, format: byte" but it should be "type: string, format: binary".
Might need to hand-edit the generated API def to fix this.

TG: Reiterating that the format of the request body should be documented.

Link to meeting recording

@slahabar
Copy link
Author

Addressed the above comments in the PR.
#15699

I updated the api description to indicate that it is for first party use and intended to be used within a SDK. This is our primary use case although it will be available to 3rd customers, they should avoid using it.
@mikekistler @johanste @tg-msft

@czubair czubair added Reviewed-ChangesRequested <valid label in PR review process>add this label when assignee request changes after review and removed APIStewardshipBoard-ReviewRequested This should be reviewed by the Azure API Stewardship team in partnership with the service team. labels Aug 31, 2021
@markweitzel markweitzel self-assigned this Sep 1, 2021
@markweitzel markweitzel linked a pull request Sep 1, 2021 that will close this issue
25 tasks
@markweitzel markweitzel added APIStewardshipBoard-SignedOff The Azure API Stewardship team has reviewed and approved the changes. and removed Reviewed-ChangesRequested <valid label in PR review process>add this label when assignee request changes after review labels Sep 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
APIStewardshipBoard-SignedOff The Azure API Stewardship team has reviewed and approved the changes.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants
@markweitzel @jhendrixMSFT @slahabar @jianyexi @czubair @mikekistler and others