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

Add OpenAPI for curator service api #226

Merged
merged 4 commits into from
Jun 4, 2020
Merged

Add OpenAPI for curator service api #226

merged 4 commits into from
Jun 4, 2020

Conversation

allysonjp715
Copy link
Contributor

@allysonjp715 allysonjp715 commented Jun 4, 2020

I wasn't sure whether or not to add /cases to the yaml, since it is already documented/verified in the data-service OpenAPI. I refrained for now, but let me know if you think it would be useful to add here as well.

Screen Shot 2020-06-04 at 11 27 57 AM

@allysonjp715 allysonjp715 requested review from axmb and khmoran June 4, 2020 10:26
@allysonjp715
Copy link
Contributor Author

Copy link
Contributor

@axmb axmb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@khmoran
Copy link
Contributor

khmoran commented Jun 4, 2020

I wasn't sure whether or not to add /cases to the yaml, since it is already documented/verified in the data-service OpenAPI. I refrained for now, but let me know if you think it would be useful to add here as well.

I guess we should? I'm guessing they'll have the flexibility to diverge at a certain point, as each service becomes more tailored to its individual purpose. But we don't have to do it now. @axmb WDYT

put:
summary: Updates a specific user's roles
operationId: updateUser
requestBody:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it looks like the body of this request is currently a list of roles, not the whole user obj: https://github.com/open-covid-data/healthmap-gdo-temp/blob/master/verification/curator-service/api/src/controllers/users.ts#L58

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yea I was thinking it's possible to pass the whole user object, and we'll only access the roles field. But it's worth making it explicit that roles is the only field we're using. Done.

@axmb
Copy link
Contributor

axmb commented Jun 4, 2020

I wasn't sure whether or not to add /cases to the yaml, since it is already documented/verified in the data-service OpenAPI. I refrained for now, but let me know if you think it would be useful to add here as well.

I guess we should? I'm guessing they'll have the flexibility to diverge at a certain point, as each service becomes more tailored to its individual purpose. But we don't have to do it now. @axmb WDYT

It should probably be documented, so that the OpenAPI page is a complete list of endpoints provided by the service. I don't think it has to be done now, either. Perhaps there's a simple way to create a section for this, and say "see <:3000/api/docs>" until they actually diverge.

Copy link
Contributor

@khmoran khmoran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice!

@allysonjp715
Copy link
Contributor Author

I wasn't sure whether or not to add /cases to the yaml, since it is already documented/verified in the data-service OpenAPI. I refrained for now, but let me know if you think it would be useful to add here as well.

I guess we should? I'm guessing they'll have the flexibility to diverge at a certain point, as each service becomes more tailored to its individual purpose. But we don't have to do it now. @axmb WDYT

It should probably be documented, so that the OpenAPI page is a complete list of endpoints provided by the service. I don't think it has to be done now, either. Perhaps there's a simple way to create a section for this, and say "see <:3000/api/docs>" until they actually diverge.

The swagger docs say you can use a $ref for a path:
Screen Shot 2020-06-04 at 6 01 43 PM

And the $ref docs say you can use a relative path to another yaml file:
Screen Shot 2020-06-04 at 6 05 10 PM

But there is a bug in openapi v3 that relative paths aren't working: swagger-api/swagger-editor#1409

That bug was assigned to someone 12 days ago, so maybe they'll make some progress there. I'll wait to see if there are any updates there and add the ref for cases here if it gets fixed.

@allysonjp715 allysonjp715 merged commit 8608257 into master Jun 4, 2020
@allysonjp715 allysonjp715 deleted the openapi branch June 4, 2020 17:29
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