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 server version in SDK #4935

Merged
merged 11 commits into from
Sep 30, 2022
Merged

Check server version in SDK #4935

merged 11 commits into from
Sep 30, 2022

Conversation

zhiltsov-max
Copy link
Contributor

@zhiltsov-max zhiltsov-max commented Sep 12, 2022 •
edited
Loading

Motivation and context

  • Allowed unauthorized access to api/about, api/docs, api/swagger, api/schema
  • Synchronized reported server API version in schema and in api/about
  • Added tests
  • Added a check in SDK and CLI for the server API version
    • Currently, versions from 2.0 to 2.3 are all considered supported
    • SDK allows to configure the desired check behaviour
  • Fixed SDK .gitignore to ignore only files in the package root. It fixes file addition from cvat-sdk/gen/
  • Improved host schema detection failure message (CLI no longer working after update #4971)

How has this been tested?

Unit tests

Checklist

License

  • I submit my code changes under the same MIT License that covers the project.
    Feel free to contact the maintainers if that's a concern.

@zhiltsov-max zhiltsov-max changed the title Check server version in SDK [WIP] Check server version in SDK Sep 16, 2022
@zhiltsov-max zhiltsov-max changed the title [WIP] Check server version in SDK Check server version in SDK Sep 20, 2022
@zhiltsov-max
Copy link
Contributor Author

@nmanovic , please check this PR.

nmanovic
nmanovic reviewed Sep 28, 2022
View reviewed changes
cvat-sdk/cvat_sdk/core/client.py
raise IncompatibleVersionException(msg)

def get_server_version(self) -> pv.Version:
# TODO: allow to use this endpoint unauthorized
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably need to remove the TODO

@nmanovic
Copy link
Contributor

/check

nmanovic
nmanovic approved these changes Sep 28, 2022
View reviewed changes
Copy link
Contributor

@nmanovic nmanovic left a comment

Choose a reason for hiding this comment

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

LGTM

@github-actions
Copy link
Contributor

github-actions bot commented Sep 28, 2022 •
edited
Loading

❌ Some checks failed
📄 See logs here

@nmanovic
Copy link
Contributor

 @zhiltsov-max , could you please look at tests?

@nmanovic nmanovic removed the request for review from azhavoro September 28, 2022 08:09
@nmanovic
Copy link
Contributor

/check

@github-actions
Copy link
Contributor

github-actions bot commented Sep 29, 2022 •
edited
Loading

❌ Some checks failed
📄 See logs here

@sizov-kirill
Copy link
Contributor

Tests are red because this branch doesn't contain last changes from develop branch, please try to merge develop branch into the zm/check-server-version-in-sdk

@cvat-ai cvat-ai deleted a comment from github-actions bot Sep 29, 2022
@nmanovic
Copy link
Contributor

/check

@github-actions
Copy link
Contributor

github-actions bot commented Sep 29, 2022 •
edited
Loading

✔️ All checks completed successfully
📄 See logs here

@nmanovic nmanovic merged commit 2e15025 into develop Sep 30, 2022
@nmanovic nmanovic deleted the zm/check-server-version-in-sdk branch September 30, 2022 18:55
@nmanovic nmanovic mentioned this pull request Nov 1, 2022
Closed
@nmanovic nmanovic mentioned this pull request Dec 12, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nmanovic nmanovic nmanovic 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
@zhiltsov-max @nmanovic @sizov-kirill