Let FastAPI perform pydantic validation of EverestConfig objects - #14326
Let FastAPI perform pydantic validation of EverestConfig objects#14326berland wants to merge 2 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #14326 +/- ##
===========================================
- Coverage 91.92% 70.65% -21.28%
===========================================
Files 485 497 +12
Lines 33655 36084 +2429
===========================================
- Hits 30939 25494 -5445
- Misses 2716 10590 +7874
Flags with carried forward coverage won't be shown. Click here to find out more.
|
The context (knowledge of installed forward model steps etc) is injected as a dependency to FastAPI. The existing behaviour of muting all ConfigWarnings in the context-aware validation is continued in this commit. This assumes that both the client and the server is executed in an identical environment.
1f12f5c to
bad4d8c
Compare
| @router.post("/" + EverEndpoints.START_EXPERIMENT, dependencies=authenticated) | ||
| @router.post( | ||
| "/" + EverEndpoints.START_EXPERIMENT, | ||
| dependencies=[*authenticated, Depends(_with_runtime_plugins)], |
There was a problem hiding this comment.
Should we folow the pattern of authenticated = [Depends(verify_auth)] for Depends(_with_runtime_plugins) as well?
There was a problem hiding this comment.
I am not sure it will add anything good, a little bit skeptical to the existing pattern of having a 1-member list for authenticated - I guess it can make sense in case this list can grow. I don't think the list of dependencies for runtime_plugins can grow.
| retries: int = 5, | ||
| ) -> str: | ||
| url, cert, auth = server_context | ||
| last_error: str | None = None |
There was a problem hiding this comment.
Why do we only want the last error message? Would it be better if we appended the messages?
There was a problem hiding this comment.
I think they are very likely to be identical. All error message can be found in the logs if needed.
| monkeypatch.setenv("ERT_STORAGE_TOKEN", "password") | ||
| credentials = b64encode(b"username:password").decode() | ||
| auth_headers = {"Authorization": f"Basic {credentials}"} | ||
| client = TestClient(app) |
There was a problem hiding this comment.
I see that this is repeated a lot, could we add a fixture or method that generates it? e.g returns tuple of credentials, auth_headers and client?
3 participants
The context (knowledge of installed forward model steps etc) is injected as a dependency to FastAPI.
The existing behaviour of muting all ConfigWarnings in the context-aware validation is continued in this commit. This assumes that both the client and the server is executed in an identical environment.
Issue
Resolves #13923
Approach
FastAPI dependencies with ExitStack
git rebase -i main --exec 'just rapid-tests')When applicable
merge screenshot-PR in ert-testdata before merging this PR.