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

✨ Increment api version #59

Merged

Conversation

bisgaard-itis
Copy link
Collaborator

What do these changes do?

Update openapi.json and expose new functionality. Also add some sanity checks to the tests.

Related issue/s

ITISFoundation/osparc-issues#668

How to test

@bisgaard-itis bisgaard-itis requested a review from pcrespov as a code owner July 19, 2023 12:12
@bisgaard-itis bisgaard-itis self-assigned this Jul 19, 2023
@bisgaard-itis bisgaard-itis added the enhancement New feature or request label Jul 19, 2023
@bisgaard-itis bisgaard-itis added this to the Sundae milestone Jul 19, 2023
Copy link
Member

@pcrespov pcrespov left a comment

Choose a reason for hiding this comment

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

Cool! Left some comments!

)
from osparc_client.models import RunningState as TaskStates
from osparc_client.models import Solver, UserRoleEnum, UsersGroup, ValidationError

__all__ = [
Copy link
Member

Choose a reason for hiding this comment

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

MINOR: use tuple instead of list for all, i.e.

__all__ : tuple[str, ...] = ( "BodyUploadFileV0FilesContentPut",  )

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

Copy link
Member

Choose a reason for hiding this comment

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

still a list?

Copy link
Collaborator Author

@bisgaard-itis bisgaard-itis Jul 20, 2023

Choose a reason for hiding this comment

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

Sorry, I managed to fix it in __init__.py but not in models.py and api.py 🤦‍♂️. That's done now.

"OnePageSolverPort",
"StudyPort",
"Study",
"LimitOffsetPageStudy",
Copy link
Member

Choose a reason for hiding this comment

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

oh! it produces really ugly model names for the pagination. Will review that later :-)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since we have anyway taken control over the user facing API by having our wrapper we could simply start wrapping these methods.

}
"title": "osparc.io web API (dev)",
"description": "osparc-simcore public API specifications",
"version": "0.4.5-dev"
Copy link
Member

Choose a reason for hiding this comment

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

How would I know to what commit of the openapi-simcore vcs this OAS belongs?
OK so it is here that we know that it is not released, right? i.e. -dev
Should we add some info int he version? SEE https://regex101.com/r/Ly7O1x/3/
https://semver.org/#is-there-a-suggested-regular-expression-regex-to-check-a-semver-string

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is a good question 🤔. I was thinking about questions closely related myself because I am more used to a tighter coupling between client and server (i.e. one where the versions have to match).

I think the point is that since the client and server are completely decoupled we should not be able to see that directly in the openapi.json. Instead we should request the openapi.json form the server and do a comparison. Hence, how about adding a test to the e2e which (through the client) downloads the openapi.json from the server and runs one of the openapi tools (e.g. the diff tool) to check that the client and server are really compatible?

This wouldn't check what I think you want to check (i.e. that all endpoints can be hit by our client), but for that I guess we have the tutorials.

What do you think?

@bisgaard-itis bisgaard-itis requested a review from pcrespov July 19, 2023 14:15
Copy link
Member

@pcrespov pcrespov left a comment

Choose a reason for hiding this comment

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

Cool! some minors

)
from osparc_client.models import RunningState as TaskStates
from osparc_client.models import Solver, UserRoleEnum, UsersGroup, ValidationError

__all__ = [
Copy link
Member

Choose a reason for hiding this comment

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

still a list?

@@ -98,16 +102,14 @@ pylint: _check_venv_active ## runs linter (only to check errors. SEE .pylintrc e
.PHONY: test-dev
test-dev: _check_venv_active ## runs tests during development
# runs tests for development (e.g w/ pdb)
pytest -vv --exitfirst --failed-first --durations=10 --pdb --ignore=$(ARTIFACTS_DIR)/client $(PYTHON_DIR)
python -m pytest -vv --exitfirst --failed-first --durations=10 --pdb $(PYTHON_DIR)/test/test_osparc
Copy link
Member

Choose a reason for hiding this comment

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

  • Good move. _check_venv_active guarantees python but not pytest executable!
  • TIP: expand options in multi-line

Copy link
Collaborator Author

@bisgaard-itis bisgaard-itis Jul 20, 2023

Choose a reason for hiding this comment

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

done

@bisgaard-itis bisgaard-itis merged commit be79c5a into ITISFoundation:master Jul 20, 2023
@bisgaard-itis bisgaard-itis deleted the increment-api-version branch July 20, 2023 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants