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

🐛 ensure backwards compatibility of api server #6866

Conversation

bisgaard-itis
Copy link
Contributor

@bisgaard-itis bisgaard-itis commented Nov 29, 2024

What do these changes do?

  • This PR aims to restore backwards compatibility of the api-server before it is released to the public. The pydantic upgrade broke backwards compatibility and this PR hope to restore it.
  • It also creates/updates a make target openapi-specs in the top-level Makefile which generates all openapi-specs within the repo. Currently there are two services which are causing issues here: Storage and Director. It seems their openapi specs are not properly created by the Make targets in the services directories.
  • It adds a test to the CI which checks that the openapi-specs are up-to-date before merging into master. This approach has several advantages:
    • The Make target for generating the specs are very slow, so not suitable for a git precommit hook.
    • With the approach chosen here, the code owner of a service will be notified when the api of his/her service changes.
  • A CI test is added which checks that backwards compatibility is not broken for any rest api within osparc-simcore before merging to master. Note this Ci test does not block merging a PR
  • A CI test which tests that the api-server does not break backwards compatibility. This one blocks merging a PR

Related issue/s

How to test

Dev-ops checklist

@bisgaard-itis
Copy link
Contributor Author

bisgaard-itis commented Nov 29, 2024

All endpoints (except the paginated ones) have been checked to be backwards compatible on the commit c72f500. This was done my running the following script from services/api-server

make openapi.json
../../scripts/openapi-diff.bash breaking https://raw.githubusercontent.com/ITISFoundation/osparc-simcore/4abb9f23e99c5cd854e348a81f8e8e4e4c88c691/services/api-server/openapi.json \
    /specs/openapi.json  \
    --flatten-allof \
    --unmatch-path "^/v0/credits/price$|^/v0/files$|^/v0/files/content$|^/v0/me$|^/v0/solvers$|^/v0/solvers/releases$|^/v0/files/{file_id}$|^/v0/files/{file_id}:complete$|^/v0/solvers/{solver_key}/latest$|^/v0/solvers/{solver_key}/releases$|^/v0/solvers/{solver_key}/releases/{version}$|^/v0/solvers/{solver_key}/releases/{version}/jobs$|^/v0/solvers/{solver_key}/releases/{version}/jobs/{job_id}$|^/v0/solvers/{solver_key}/releases/{version}/jobs/{job_id}/metadata$|^/v0/solvers/{solver_key}/releases/{version}/jobs/{job_id}/outputs$|^/v0/solvers/{solver_key}/releases/{version}/jobs/{job_id}/wallet$|^/v0/solvers/{solver_key}/releases/{version}/jobs/{job_id}:start$|^/v0/solvers/{solver_key}/releases/{version}/jobs/{job_id}:inspect$|^/v0/solvers/{solver_key}/releases/{version}/jobs/{job_id}:stop$|^/v0/studies/{study_id}$|^/v0/studies/{study_id}/jobs$|^/v0/studies/{study_id}/jobs/{job_id}/metadata$|^/v0/studies/{study_id}/jobs/{job_id}/outputs$|^/v0/studies/{study_id}/jobs/{job_id}:inspect$|^/v0/studies/{study_id}/jobs/{job_id}:start$|^/v0/studies/{study_id}/jobs/{job_id}:stop$|^/v0/studies/{study_id}:clone$|^/v0/wallets/default$|^/v0/wallets/{wallet_id}$"

The unmatched paths in this command are there due to a bug in pydantic v1 or an earlier version of openapi. The point is that the openapi specs changed for all these endpoints, but the actual endpoints didn't. I.e. previously the specs didn't match the actual API 💩. I checked manually that these endpoints did not actually break backwards compatibility. The endpoints which are still to be checked are the following paginated endpoints:

  • GET /v0/files:search
  • GET /v0/solvers/{solver_key}/releases/{version}/jobs/page
  • GET /v0/solvers/{solver_key}/releases/{version}/ports
  • GET /v0/studies
  • GET /v0/studies/{study_id}/ports
    In order to check these I will need your help @pcrespov because I still don't fully understand how the pagination works.

Conclusion: In order to ensure backwards compatibility of further changes can be done against commit c72f500

Copy link

codecov bot commented Nov 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.35%. Comparing base (1f9c110) to head (62955cd).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6866      +/-   ##
==========================================
- Coverage   88.34%   86.35%   -1.99%     
==========================================
  Files        1568      902     -666     
  Lines       61498    40074   -21424     
  Branches     2001      262    -1739     
==========================================
- Hits        54331    34607   -19724     
+ Misses       6832     5407    -1425     
+ Partials      335       60     -275     
Flag Coverage Δ
integrationtests 65.09% <ø> (+0.02%) ⬆️
unittests 87.05% <100.00%> (+0.51%) ⬆️
Components Coverage Δ
api 76.84% <ø> (-1.58%) ⬇️
pkg_aws_library ∅ <ø> (∅)
pkg_dask_task_models_library ∅ <ø> (∅)
pkg_models_library ∅ <ø> (∅)
pkg_notifications_library ∅ <ø> (∅)
pkg_postgres_database ∅ <ø> (∅)
pkg_service_integration ∅ <ø> (∅)
pkg_service_library ∅ <ø> (∅)
pkg_settings_library ∅ <ø> (∅)
pkg_simcore_sdk 77.37% <ø> (-8.02%) ⬇️
agent ∅ <ø> (∅)
api_server 90.03% <100.00%> (+0.17%) ⬆️
autoscaling ∅ <ø> (∅)
catalog 90.57% <ø> (ø)
clusters_keeper ∅ <ø> (∅)
dask_sidecar ∅ <ø> (∅)
datcore_adapter ∅ <ø> (∅)
director 76.40% <ø> (ø)
director_v2 91.37% <ø> (ø)
dynamic_scheduler 97.07% <ø> (ø)
dynamic_sidecar 59.86% <ø> (-29.89%) ⬇️
efs_guardian ∅ <ø> (∅)
invitations ∅ <ø> (∅)
osparc_gateway_server ∅ <ø> (∅)
payments 92.65% <ø> (ø)
resource_usage_tracker 89.07% <ø> (+0.29%) ⬆️
storage ∅ <ø> (∅)
webclient ∅ <ø> (∅)
webserver 88.33% <ø> (-0.02%) ⬇️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1f9c110...62955cd. Read the comment docs.

@bisgaard-itis
Copy link
Contributor Author

All endpoints (except the paginated ones) have been checked to be backwards compatible on the commit c72f500. This was done my running the following script from services/api-server

make openapi.json
../../scripts/openapi-diff.bash breaking https://raw.githubusercontent.com/ITISFoundation/osparc-simcore/4abb9f23e99c5cd854e348a81f8e8e4e4c88c691/services/api-server/openapi.json \
    /specs/openapi.json  \
    --flatten-allof \
    --unmatch-path "^/v0/credits/price$|^/v0/files$|^/v0/files/content$|^/v0/me$|^/v0/solvers$|^/v0/solvers/releases$|^/v0/files/{file_id}$|^/v0/files/{file_id}:complete$|^/v0/solvers/{solver_key}/latest$|^/v0/solvers/{solver_key}/releases$|^/v0/solvers/{solver_key}/releases/{version}$|^/v0/solvers/{solver_key}/releases/{version}/jobs$|^/v0/solvers/{solver_key}/releases/{version}/jobs/{job_id}$|^/v0/solvers/{solver_key}/releases/{version}/jobs/{job_id}/metadata$|^/v0/solvers/{solver_key}/releases/{version}/jobs/{job_id}/outputs$|^/v0/solvers/{solver_key}/releases/{version}/jobs/{job_id}/wallet$|^/v0/solvers/{solver_key}/releases/{version}/jobs/{job_id}:start$|^/v0/solvers/{solver_key}/releases/{version}/jobs/{job_id}:inspect$|^/v0/solvers/{solver_key}/releases/{version}/jobs/{job_id}:stop$|^/v0/studies/{study_id}$|^/v0/studies/{study_id}/jobs$|^/v0/studies/{study_id}/jobs/{job_id}/metadata$|^/v0/studies/{study_id}/jobs/{job_id}/outputs$|^/v0/studies/{study_id}/jobs/{job_id}:inspect$|^/v0/studies/{study_id}/jobs/{job_id}:start$|^/v0/studies/{study_id}/jobs/{job_id}:stop$|^/v0/studies/{study_id}:clone$|^/v0/wallets/default$|^/v0/wallets/{wallet_id}$"

The unmatched paths in this command are there due to a bug in pydantic v1 or an earlier version of openapi. The point is that the openapi specs changed for all these endpoints, but the actual endpoints didn't. I.e. previously the specs didn't match the actual API 💩. I checked manually that these endpoints did not actually break backwards compatibility. The endpoints which are still to be checked are the following paginated endpoints:

* [x]  GET /v0/files:search

* [x]  GET /v0/solvers/{solver_key}/releases/{version}/jobs/page

* [x]  GET /v0/solvers/{solver_key}/releases/{version}/ports

* [x]  GET /v0/studies

* [x]  GET /v0/studies/{study_id}/ports
  In order to check these I will need your help @pcrespov because I still don't fully understand how the pagination works.

Conclusion: In order to ensure backwards compatibility of further changes can be done against commit c72f500

Update: These last remaining endpoints were checked together with Pedro.

Copy link
Contributor

@matusdrobuliak66 matusdrobuliak66 left a comment

Choose a reason for hiding this comment

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

Thanks!

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.

The new step is going to be very useful

Copy link

sonarqubecloud bot commented Dec 6, 2024

@bisgaard-itis bisgaard-itis enabled auto-merge (squash) December 6, 2024 09:08
@sanderegg sanderegg disabled auto-merge December 6, 2024 09:22
@sanderegg sanderegg merged commit 35f11df into ITISFoundation:master Dec 6, 2024
87 of 92 checks passed
@bisgaard-itis bisgaard-itis deleted the 6865-ensure-backwards-compatibility-of-api-server branch December 6, 2024 09:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
t:maintenance Some planned maintenance work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants