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

Improve docs on exports API endpoints #14224

Merged
merged 2 commits into from
Oct 9, 2024

Conversation

SpangleLabs
Copy link
Contributor

Proposed change

  • Adding comments about the optional "name" parameter for the API endpoint to create a new export.
  • Adding an entry in the documentation about the /api/exports endpoint

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New feature
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code (docs)
    (Not sure if docs should be a separate type here, or whether this counts as new feature, for documenting features without docs, or bugfix for adding docs where they perhaps should have existed)

Additional information

I was unsure whether this merges into dev or master, but I saw other PRs merged into master when updating docs (for example #14202) and dev branch will be headed towards post-0.14 changes now I suppose.
Originally I was planning to add an issue asking why these features weren't available in the API, but then I checked the API source and found they do exist.

One question I do have is that it seems like the /api/exports endpoint lists the video path relative to the container root, rather than relative to the web root. Is that intended? Should a user of that API strip "/media/frigate" from the video path, or that they should just use the export ID as {frigate_url}/exports/{export_id}.mp4

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • The code has been formatted using Ruff (ruff format frigate)
    (Not applicable for the docs directory?)

Screenshot:
image

Copy link

netlify bot commented Oct 8, 2024

Deploy Preview for frigate-docs ready!

Name Link
🔨 Latest commit 83d64a9
🔍 Latest deploy log https://app.netlify.com/sites/frigate-docs/deploys/67057b8258bebe0008d77125
😎 Deploy Preview https://deploy-preview-14224--frigate-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@NickM-27
Copy link
Sponsor Collaborator

NickM-27 commented Oct 8, 2024

Worth noting that in the dev branch the API docs are now generated automatically, this can be merged but it will be fixed automatically when 0.15 is released

Is that intended? Should a user of that API strip "/media/frigate" from the video path

Yes

@SpangleLabs
Copy link
Contributor Author

Worth noting that in the dev branch the API docs are now generated automatically, this can be merged but it will be fixed automatically when 0.15 is released

Oh neat! I see, docusaurus will be generating pages from the openAPI spec, that'll be cool for a lot of reasons.

Though I guess the json request and response body stuff still needs filling out for those:
image
image

I pondered whether I can add that too, while I'm digging around, but I can't see what actually generates that openAPI spec file though, is it generated by the fastAPI definitions, or is that part not yet defined? Or is it intended to be manually modifying the openapi spec file?

@NickM-27
Copy link
Sponsor Collaborator

NickM-27 commented Oct 8, 2024

it is generated by fastapi, though I believe the contributor that implemented fastapi has already planned to work out building out the schema of responses.

@SpangleLabs
Copy link
Contributor Author

Oh, awesome! Yeah, I see it's mentioned on #14178, cool!

Alright, I won't worry about digging through that then. This fix should be fine for 0.14 at least

@hawkeye217 hawkeye217 merged commit f86957e into blakeblackshear:master Oct 9, 2024
10 checks passed
@iursevla
Copy link
Contributor

iursevla commented Oct 14, 2024

@SpangleLabs I'm currently updating the requetsts/responses to have better/more information 🙏

I will be working on this for 1-2 weeks and every endpoint should have a better documentation

If you find any incongruency please let me know. Whenever I open a new PR if I remember I'll tag you

I pondered whether I can add that too, while I'm digging around, but I can't see what actually generates that openAPI spec file though, is it generated by the fastAPI definitions, or is that part not yet defined? Or is it intended to be manually modifying the openapi spec file?

Basically, now FastAPI generates the json spec (which for now, we convert manually - this might be an automation on the future - but for now there's no need to do it) and then docusaurus openAPI plugin generates the docs from the spec file.

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.

4 participants