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

OpenApi schema test #143

Merged
merged 5 commits into from
Jul 31, 2023
Merged

OpenApi schema test #143

merged 5 commits into from
Jul 31, 2023

Conversation

enadeau
Copy link
Contributor

@enadeau enadeau commented Jul 28, 2023

  • fix version comparison in skip condition for open api schema validation
  • upgrade fastapi to 0.100.1

@enadeau
Copy link
Contributor Author

enadeau commented Jul 28, 2023

I've tried backing off with the fastapi version upgrade because I thought that was causing the mypy issue and didn't want to get it mixed up with this simple fix. It seems however that something else is causing it. Doesn't seems related to this PR

@JonasKs
Copy link
Member

JonasKs commented Jul 28, 2023

I believe the MyPy issues has been resolved in another PR.

Thank you so much for this - I'll check all PRs out on Monday, and hopefully have a release ready early next week 😊

@enadeau
Copy link
Contributor Author

enadeau commented Jul 29, 2023

I figured out the cause was the version of pydantic used in the pre-commit hook. It automatically grabs the latest version which is now v2 but the library is not yet compatible. I'm pinning it for now and the compatibility issue can be resolved in an other PR.

@codecov
Copy link

codecov bot commented Jul 30, 2023

Codecov Report

Merging #143 (eeb768e) into main (e8bfae5) will not change coverage.
The diff coverage is 100.0%.

@@           Coverage Diff           @@
##             main     #143   +/-   ##
=======================================
  Coverage   100.0%   100.0%           
=======================================
  Files           6        6           
  Lines         247      247           
=======================================
  Hits          247      247           
Files Changed Coverage Δ
fastapi_azure_auth/auth.py 100.0% <100.0%> (ø)

@enadeau
Copy link
Contributor Author

enadeau commented Jul 30, 2023

The pydantic compatibility issue are addressed in #144. We should probably merge it first so that we don't need to do any pinning here. I think it should also fix the failure that we see here with 3.12.

All problem in #142 are addressed. I needed to modify the name of the scheme. I've made something up, but it'll be good to hear what you think.

@JonasKs
Copy link
Member

JonasKs commented Jul 31, 2023

Could you rebase main? I'll make sure to get a release ASAP. Thank you so much for these!

@enadeau
Copy link
Contributor Author

enadeau commented Jul 31, 2023

I've rebased but the spec with pydantic v2 is slightly different from the one with v1 causing all the v2 run to fail.

I'm not quite sure what's the best solution here. We could either make another test specific to v2 schema or remove the test considering that validating the spec is good enough for us. Any thought on that?

@JonasKs
Copy link
Member

JonasKs commented Jul 31, 2023

I think we can just default to v2 here? And skip if version is 1.?

@enadeau
Copy link
Contributor Author

enadeau commented Jul 31, 2023

I just did that. Good to go now

@JonasKs JonasKs merged commit 474e14c into Intility:main Jul 31, 2023
16 checks passed
@JonasKs
Copy link
Member

JonasKs commented Jul 31, 2023

Thank you so much again, @enadeau 😊

@enadeau enadeau deleted the fix/openapi-scheme-test branch July 31, 2023 10:51
nikstuckenbrock pushed a commit to nikstuckenbrock/fastapi-azure-auth that referenced this pull request Oct 16, 2023
* fix version comparison in skip condition

* openapi_version based on fastapi version

* add test to validate openapi spec

* Make the generated openapi spec 3.1 compliant

- Fix two operation that had the same operation id
- Make the security scheme follow the pattern ^[a-zA-Z0-9._-]+$

* only test schema for version 2 of pydantic

---------

Co-authored-by: Émile Nadeau <emile.nadeau@ruv.is>
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.

2 participants