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

refactor: revamp add OpenAPI route #979

Merged
merged 1 commit into from
Oct 12, 2024

Conversation

kigawas
Copy link
Contributor

@kigawas kigawas commented Oct 8, 2024

Description

This PR fixes

  • OpenAPI routes should be added after _check_port
  • logger.info("Docs hosted at http://%s:%s/docs", host, port) was not correct. It just ignored ROBYN_HOST and ROBYN_PORT

Summary

  • Extract adding OpenAPI routes
  • Add auth_required argument for future implementation
  • Adjust the order to output the correct log

PR Checklist

Please ensure that:

  • The PR contains a descriptive title
  • The PR contains a descriptive summary of the changes
  • You build and test your changes before submitting a PR.
  • You have added relevant documentation
  • You have added relevant tests. We prefer integration tests wherever possible

Pre-Commit Instructions:

Copy link

vercel bot commented Oct 8, 2024

@kigawas is attempting to deploy a commit to the sparckles Team on Vercel.

A member of the Team first needs to authorize it.

@sansyrox
Copy link
Member

Hey @kigawas 👋

Thanks for the PR 😄 One follow up - how did you find this issue?

@kigawas
Copy link
Contributor Author

kigawas commented Oct 11, 2024

@sansyrox
I was setting ROBYN_PORT directly and the log's port was different

@sansyrox sansyrox changed the title Revamp add OpenAPI route refactor: revamp add OpenAPI route Oct 12, 2024
Copy link
Member

@sansyrox sansyrox left a comment

Choose a reason for hiding this comment

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

Looks good to me 😄

@sansyrox sansyrox merged commit 6cd93ac into sparckles:main Oct 12, 2024
56 of 58 checks passed
@kigawas kigawas deleted the fix-add-openapi-route branch October 13, 2024 14:14
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