Skip to content

Conversation

amites
Copy link
Contributor

@amites amites commented Jul 1, 2021

This PR closes #257

What does this PR do?

Creates OpenAPI doc & some grooming

How does this PR make you feel?

![good]

@netlify
Copy link

netlify bot commented Jul 1, 2021

👷 Deploy request for upswyng accepted.

🔨 Explore the source changes: 9fa4b74

🔍 Inspect the deploy log: https://app.netlify.com/sites/upswyng/deploys/60f8ce28a61dfa00076f1d18

Copy link
Member

@jollyjerr jollyjerr left a comment

Choose a reason for hiding this comment

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

I think this approach is solid. Do we want to add all docs as part of this pr or break it up into multiple?

@amites
Copy link
Contributor Author

amites commented Jul 2, 2021

I think this approach is solid. Do we want to add all docs as part of this pr or break it up into multiple?

I'll update this PR and migrate changes to the README and support for waiting for docker to be ready into their own branch to keep things clean

@amites
Copy link
Contributor Author

amites commented Jul 6, 2021

heavily updated -- reverted everything not the docs file

docs file has complete or stubs for nearly every endpoint

@amites amites marked this pull request as ready for review July 10, 2021 20:21
@amites amites requested a review from rhinodavid as a code owner July 10, 2021 20:21
@@ -4,7 +4,6 @@ import {
TLegacyResource,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed unused import & fix typo

@@ -6,7 +6,7 @@ import mq from "../../../worker/mq";
async function makeTestJob(req, res, _next, user: TUser) {
const jobName = getName("-");

// Command can be like /testjob 35, where the second part is the delay to apply to the job
// Command can be like /test-job 35, where the second part is the delay to apply to the job
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the actual command includes the -

@@ -12,6 +13,19 @@ export async function get(req, res, _next) {
"Content-Type": "application/json",
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this matches the singleton and other draft responses

Copy link
Member

@jacobvenable jacobvenable left a comment

Choose a reason for hiding this comment

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

A few minor issues, but overall looks really good! For maintaining these, I wonder there's a tool that can create schemas based off of TS types. That could make managing these a bit simpler. Awesome job!

Copy link
Member

@jollyjerr jollyjerr left a comment

Choose a reason for hiding this comment

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

Wow, this looks like a lot of work! I think this will be in a good place once we can address Jacob's feedback. As for generating schemas via type definitions - a quick search did not find a good tool, but one may exist. We also could just define the return schema by referencing the type when appropriate? Like Code: 200, Description: TResource[] & { totalFound: 800 } etc.... This would add an extra step for an engineer to need to go reference the type - but I almost prefer that over having two sources of truth?

@amites
Copy link
Contributor Author

amites commented Jul 22, 2021

updated based on comments from @jacobvenable thank you for the detailed review

in terms of keeping them up to date -- sadly there isn't much out of the box available as @jollyjerr found

there's a lot that could be done with a custom script -- if that's a priority I'll tackle that next vs the default sorting

@jacobvenable
Copy link
Member

updated based on comments from @jacobvenable thank you for the detailed review

in terms of keeping them up to date -- sadly there isn't much out of the box available as @jollyjerr found

there's a lot that could be done with a custom script -- if that's a priority I'll tackle that next vs the default sorting

Yep that works for me 👍 One tool I had briefly looked into was tsoa. Seems nifty but is an extra layer of maintenance for our endpoints.

Copy link
Member

@jacobvenable jacobvenable left a comment

Choose a reason for hiding this comment

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

Changes LGTM. Thanks for tackling this 🌮 🚀

@jacobvenable jacobvenable merged commit ebd9dcf into CodeForBoulder:master Jul 23, 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.

[server] Document API Endpoints
3 participants