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.yaml to the repository. #3633

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

rickwysocki
Copy link

@rickwysocki rickwysocki commented Mar 2, 2024

I created an openapi.yaml spec file for Octobox. I relied both on existing documentation and my own testing of the endpoints in a Docker version of the application. I have a points/questions to review:

  • I don't think I was able to get the sync/syncing endpoints, in the notifications controller, to work in my testing. Sync (/api/notifications/sync.json) might have worked since, based on the existing documentation, there wouldn't be a response, but I couldn't find any evidence of it working. Syncing (/api/notifications/syncing.json) only ever returned full HTML pages in Terminal. I may have misinterpreted elements of the existing documentation here.
  • I couldn't seem to post or patch pinned searches. Both returned HTML pages in Terminal. Additionally, the response examples in the existing documentation confused me, so I don't think the responses are currently correct. I'd love to work more on this if you have any thoughts.
  • I've left a few other in-text comments for smaller points, such as info I wasn't sure of or didn't have.

This was a lot of fun! I'd love to keep working on this with your feedback on the current version.

EDIT: One last point I forgot to mention: I was unsure where the openapi.yaml file should go, so it is in the root directory.

@andrew
Copy link
Member

andrew commented Mar 3, 2024

Looking good, thanks for tackling this, I've just merged an update that fixes running octobox via docker-compose, so that may solve some of the issues you were running into.

@rickwysocki
Copy link
Author

Thanks for the update! I verified that Swagger's curl command works for api/notifications/sync.json, but I'm still getting responses of very large HTML/Ruby files when using the commands produced for syncing, posting pinned searches, and updating pinned searches. There don't seem to be any changes made to the local version of Octobox I'm running using Docker, either. I'll take another look tomorrow, but any advice would be helpful.

I'm happy to work on this as long as is useful, by the way, as I'm trying to get my API documentation chops together.

@andrew
Copy link
Member

andrew commented Mar 3, 2024

It could well be broken, although all the tests are passing which makes me think it’s a config issue, I’ll also give them a try tomorrow.

This is really helpful, thanks for contributing

@rickwysocki
Copy link
Author

Hi there: Just an update that I tried those commands using my live Octobox URL rather than Docker. While this didn't return HTML in terminal, I still wasn't able to post or patch pinned notifications using CURL. Additionally, I'm also still not sure whether syncing feature is working or not.

@rickwysocki
Copy link
Author

Hi there! Just following up to ask if you'd had a chance to look at those endpoints and/or any instructions for to continuing working on the API documentation.

@andrew
Copy link
Member

andrew commented Mar 14, 2024

Sorry for the delay, I’ll review this later tonight

openapi.yaml Outdated Show resolved Hide resolved
openapi.yaml Outdated Show resolved Hide resolved
openapi.yaml Show resolved Hide resolved
openapi.yaml Show resolved Hide resolved
openapi.yaml Outdated Show resolved Hide resolved
openapi.yaml Outdated Show resolved Hide resolved
openapi.yaml Outdated Show resolved Hide resolved
openapi.yaml Outdated Show resolved Hide resolved
openapi.yaml Outdated Show resolved Hide resolved
openapi.yaml Outdated Show resolved Hide resolved
openapi.yaml Outdated Show resolved Hide resolved
openapi.yaml Show resolved Hide resolved
@andrew
Copy link
Member

andrew commented Mar 14, 2024

I've added some inline comments, it's looking good so far, the schemas could do with a bit of tweaking to seperate the different objects and use the $ref feature of openapi.

Example: https://github.com/ecosyste-ms/packages/blob/main/openapi/api/v1/openapi.yaml#L1103

I could get all of the api endpoints working, a couple return 200 instead of 204, the creating and updating of pinned searches is a bit confusing as it follows the rails convention for parameters, I left examples in the comments above.

@andrew
Copy link
Member

andrew commented Mar 14, 2024

I also opened up the file in https://editor.swagger.io, all working and no errors!

Screenshot 2024-03-14 at 21-33-44 Swagger Editor

@octobox octobox deleted a comment from ger1964jou Mar 15, 2024
@rickwysocki
Copy link
Author

Thanks for this feedback! It's really helpful on my end. I'll start working on your recommendations and check in once I've finished the revisions.

rickwysocki and others added 4 commits March 16, 2024 15:33
Co-authored-by: Andrew Nesbitt <andrewnez@gmail.com>
Co-authored-by: Andrew Nesbitt <andrewnez@gmail.com>
Co-authored-by: Andrew Nesbitt <andrewnez@gmail.com>
@rickwysocki
Copy link
Author

I've made some revisions to the openapi.yaml file based on your helpful recommendations. The only things I still seem to have issues with are the two sync-related endpoints.

  1. For the api/notifications/sync.json endpoint, I get a 200 response with no body in Swagger. In Terminal, I seem to be getting a 400 error when I run a verbose CURL request produced by Swagger.
  2. For the api/notifications/syncing.json endpoint, I get a 200 response in both Swagger and Terminal, but it comes with an "error": null.

I've attached screenshots showing those errors on my end. Everything else seems to be working for me, and I've made your requested changes.


image
image

@rickwysocki
Copy link
Author

Hi there: Just touching base to see whether there is more work I can do on this. No rush on my end!

@andrew
Copy link
Member

andrew commented Apr 12, 2024

@rickwysocki nothing more needed from your right now, I'm travelling for work at the moment, hope to get back to this in about a week

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.

None yet

2 participants