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.

Sign up for GitHub

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

Jump to bottom

feat: send client version to server #267

Merged
merged 3 commits into from
Oct 18, 2024
Merged

feat: send client version to server #267

merged 3 commits into from
Oct 18, 2024

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 👍

@natthan-pigoux natthan-pigoux force-pushed the feat/send_client_version_to_server branch from 963941f to 1551b0c Compare September 23, 2024 13:37
@fstagni fstagni linked an issue Oct 3, 2024 that may be closed by this pull request
Make the client send its version when calling the server #258
Closed
@chaen chaen force-pushed the feat/send_client_version_to_server branch from 1551b0c to cbca461 Compare October 18, 2024 09:02
chaen
chaen reviewed Oct 18, 2024
View reviewed changes
diracx-routers/src/diracx/routers/version.py Outdated
@@ -0,0 +1 @@
DIRACX_MIN_CLIENT_VERSION = "0.0.1"
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer to have that constant in routers/__init__.py. routers.version is too misleading as a name I believe

chaen
chaen reviewed Oct 18, 2024
View reviewed changes
diracx-routers/src/diracx/routers/__init__.py

def get_min_client_version():
"""Extracting min client version from entry_points and searching for extension."""
matched_entry_points: EntryPoints = entry_points(group="diracx.min_client_version")
Copy link
Contributor

Choose a reason for hiding this comment

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

This just made me realize that it is only compatible with versioning the vanilla diracx client, and not the extension. You already spent a substantial amount of time on this PR, so I will not ask you to make it extension-compatible 😆 I'll just open an issue with it

@chaen chaen merged commit 00665bd into DIRACGrid:main Oct 18, 2024
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aldbr aldbr aldbr approved these changes

@chaen chaen chaen approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make the client send its version when calling the server
3 participants
@natthan-pigoux @aldbr @chaen