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

Check for version compatibility on login #329

Merged
merged 1 commit into from
Nov 22, 2023
Merged

Check for version compatibility on login #329

merged 1 commit into from
Nov 22, 2023

Conversation

gregorjerse
Copy link
Contributor

@gregorjerse gregorjerse commented Oct 28, 2023 •
edited
Loading

EXP-778

dblenkus
dblenkus reviewed Oct 30, 2023
View reviewed changes
src/resdk/resolwe.py Outdated
@@ -48,6 +50,7 @@
DEFAULT_URL = "http://localhost:8000"
AUTOMATIC_LOGIN_POSTFIX = "saml-auth/api-login/"
INTERACTIVE_LOGIN_POSTFIX = "saml-auth/remote-login/"
MINIMAL_SUPPORTED_VERSION_POSTFIX = "api/reskd_minimal_supported_version"
Copy link
Member

Choose a reason for hiding this comment

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

Typo in the URL.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

src/resdk/resolwe.py Outdated
response = requests.get(url)
minimal_version = version.parse(response.data["minimal_supported_version"])
my_version = version.parse(package_version("resdk"))
# TODO: probably some warning must be printed here?
Copy link
Member

Choose a reason for hiding this comment

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

Remove the comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

src/resdk/resolwe.py Outdated
# TODO: probably some warning must be printed here?
if my_version < minimal_version:
message = (
f"Warning: your version of ReSDK ({my_version}) is not compatible with "
Copy link
Member

Choose a reason for hiding this comment

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

Add steps to update the package.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

JureZmrzlikar
JureZmrzlikar reviewed Nov 6, 2023
View reviewed changes
Copy link
Member

@JureZmrzlikar JureZmrzlikar left a comment

Choose a reason for hiding this comment

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

I agree with comments from @dblenkus . Also, let's test this when the endpoint becomes operational on QA.

@gregorjerse gregorjerse force-pushed the feature/minimal_supported_version branch 2 times, most recently from 57f7488 to 429e836 Compare November 6, 2023 09:46
JureZmrzlikar
JureZmrzlikar reviewed Nov 6, 2023
View reviewed changes
src/resdk/resolwe.py Outdated
@@ -48,6 +50,7 @@
DEFAULT_URL = "http://localhost:8000"
AUTOMATIC_LOGIN_POSTFIX = "saml-auth/api-login/"
INTERACTIVE_LOGIN_POSTFIX = "saml-auth/remote-login/"
MINIMAL_SUPPORTED_VERSION_POSTFIX = "api/resdk_minigmal_supported_version"
Copy link
Member

Choose a reason for hiding this comment

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

New typo (minigmal)
:-)

Copy link
Member

Choose a reason for hiding this comment

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

Fix this typo.

JureZmrzlikar
JureZmrzlikar reviewed Nov 6, 2023
View reviewed changes
src/resdk/resolwe.py Outdated
def version_check(self):
"""Check that the server is compatible with the client."""
url = urljoin(self.url, MINIMAL_SUPPORTED_VERSION_POSTFIX)
response = requests.get(url)
Copy link
Member

Choose a reason for hiding this comment

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

Does the code below work if case the response does not contain data or it does not have 200-ish HTTP response code?

JureZmrzlikar
JureZmrzlikar requested changes Nov 21, 2023
View reviewed changes
Copy link
Member

@JureZmrzlikar JureZmrzlikar left a comment

Choose a reason for hiding this comment

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

Two small fixes and we are good to go.

src/resdk/resolwe.py Outdated
@@ -48,6 +50,7 @@
DEFAULT_URL = "http://localhost:8000"
AUTOMATIC_LOGIN_POSTFIX = "saml-auth/api-login/"
INTERACTIVE_LOGIN_POSTFIX = "saml-auth/remote-login/"
MINIMAL_SUPPORTED_VERSION_POSTFIX = "api/resdk_minigmal_supported_version"
Copy link
Member

Choose a reason for hiding this comment

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

Fix this typo.

src/resdk/resolwe.py Outdated
"""Check that the server is compatible with the client."""
url = urljoin(self.url, MINIMAL_SUPPORTED_VERSION_POSTFIX)
response = requests.get(url)
minimal_version = version.parse(response.data["minimal_supported_version"])
Copy link
Member

Choose a reason for hiding this comment

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

This currently fails with: AttributeError: 'Response' object has no attribute 'data'

If you replace response.data with response.json() it works as expected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JureZmrzlikar Thank you for testing. I have changed the code according to your suggestions.

@gregorjerse gregorjerse force-pushed the feature/minimal_supported_version branch from 429e836 to 2904240 Compare November 22, 2023 14:51
@gregorjerse gregorjerse force-pushed the feature/minimal_supported_version branch from 2904240 to f08867b Compare November 22, 2023 14:55
@gregorjerse gregorjerse force-pushed the feature/minimal_supported_version branch 3 times, most recently from 8745770 to 0fc4eff Compare November 22, 2023 20:25
@gregorjerse gregorjerse force-pushed the feature/minimal_supported_version branch from 0fc4eff to 62ecc31 Compare November 22, 2023 20:32
@gregorjerse gregorjerse merged commit 62ecc31 into genialis:master Nov 22, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JureZmrzlikar JureZmrzlikar JureZmrzlikar approved these changes

@dblenkus dblenkus dblenkus 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.
3 participants
@gregorjerse @dblenkus @JureZmrzlikar