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

feat: send client version to server #267

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

natthan-pigoux
Copy link
Contributor

  • create a middleware to check that the client version pass in header is recent enough from what is defined in the entry_point

@natthan-pigoux
Copy link
Contributor Author

closes #258

@natthan-pigoux natthan-pigoux marked this pull request as draft June 27, 2024 14:03
@natthan-pigoux
Copy link
Contributor Author

  • Need to check why the send or send_request client method are not used to send the request.
  • It looks like the request goes through: client/aio/operations/_operations.py > self._client._pipeline.run when testing with userinfo

@natthan-pigoux natthan-pigoux force-pushed the feat/send_client_version_to_server branch 3 times, most recently from a2aee1f to d2ab06f Compare July 12, 2024 15:51
@natthan-pigoux
Copy link
Contributor Author

  • Need to check why the send or send_request client method are not used to send the request.
  • It looks like the request goes through: client/aio/operations/_operations.py > self._client._pipeline.run when testing with userinfo

@chaen find how to add properly the header in the client:
kwargs.setdefault("base_headers", {})[DiracX-Client-Version"] = self.client_version

@natthan-pigoux
Copy link
Contributor Author

Do I need to add more unit tests ?

@natthan-pigoux natthan-pigoux marked this pull request as ready for review July 16, 2024 08:02
@aldbr
Copy link
Contributor

aldbr commented Jul 16, 2024

Do I need to add more unit tests ?

  • a test when the header is missing? (which might be the case if we interact with the server using the openapi or the web interface)
  • a test when the header is present but does not contain any value?
  • a test when something other than a version is provided? Making sure the server correctly handles weird strings or numbers and does not crash.

@natthan-pigoux natthan-pigoux marked this pull request as draft July 16, 2024 13:06
@natthan-pigoux
Copy link
Contributor Author

Do I need to add more unit tests ?

  • a test when the header is missing? (which might be the case if we interact with the server using the openapi or the web interface)
  • a test when the header is present but does not contain any value?
  • a test when something other than a version is provided? Making sure the server correctly handles weird strings or numbers and does not crash.

last commits add these tests: 92fac94

@natthan-pigoux natthan-pigoux marked this pull request as ready for review July 17, 2024 14:16
@natthan-pigoux natthan-pigoux force-pushed the feat/send_client_version_to_server branch from 7450962 to 672df6a Compare September 16, 2024 14:15
@natthan-pigoux natthan-pigoux force-pushed the feat/send_client_version_to_server branch from 2b47b6e to 5b55e88 Compare September 18, 2024 09:15
@natthan-pigoux
Copy link
Contributor Author

Please tell me when review is ok, so that I can squash the commits.

@aldbr
Copy link
Contributor

aldbr commented Sep 23, 2024

Alright, you can squash the commits, it looks good 👍

- add an entry point for min client version
- create a middleware to verify server/client version
@natthan-pigoux natthan-pigoux force-pushed the feat/send_client_version_to_server branch from 963941f to 1551b0c Compare September 23, 2024 13:37
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