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

♻️ Maintenance/refactors servicelib #2516

Merged

Conversation

pcrespov
Copy link
Member

@pcrespov pcrespov commented Aug 31, 2021

What do these changes do?

  • adds extra aiohttp to separate dependencies exclusive to aiohttp-services
  • can install and test using minimal installation (e.g. for fastapi-based services) or w/ aiohttp extra (for aiohttp-based services)

In detail

  • adds extra requirements aiohttp (affects requirements/* and setup.py)
  • moves aiohttp-specific modules inside src/servicelib/aiohttp
  • moves aiohttp-specific tests inside tests/aiohttp
  • changes in Makefile

Related issue/s

How to test

make devenv
source .venv/bin/activate
cd packages/services-library

# installs normally (i.e. no aiohttp extras)
make install-dev
make test-dev

# install with aiohttp extras
make "install-dev[aiohttp]"
make "test[aiohttp]"

Checklist

  • CI two tests: with aiohttp extras and without aiohttp extras
  • adapt services using servicelib:
    • aiohttp-based: web-server and storage - install as service-library[aiohttp]
    • fastapi-based: - install as service-library
  • increase version of servicelib
  • simcore-sdk now only depends on service-library
  • run make reqs on all involved dependencies to make sure it works correctly

@pcrespov pcrespov self-assigned this Aug 31, 2021
@pcrespov pcrespov added the a:services-library issues on packages/service-libs label Aug 31, 2021
@pcrespov pcrespov added this to the Chevrotain milestone Aug 31, 2021
@codecov
Copy link

codecov bot commented Aug 31, 2021

Codecov Report

Merging #2516 (186167a) into master (7382c4e) will decrease coverage by 0.0%.
The diff coverage is 97.9%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #2516     +/-   ##
========================================
- Coverage    76.6%   76.6%   -0.1%     
========================================
  Files         609     610      +1     
  Lines       23388   23397      +9     
  Branches     2294    2294             
========================================
+ Hits        17926   17928      +2     
- Misses       4857    4865      +8     
+ Partials      605     604      -1     
Flag Coverage Δ
integrationtests 66.5% <98.3%> (+<0.1%) ⬆️
unittests 70.6% <97.9%> (-0.1%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...vice-library/src/servicelib/aiohttp/application.py 0.0% <ø> (ø)
...library/src/servicelib/aiohttp/application_keys.py 100.0% <ø> (ø)
...ibrary/src/servicelib/aiohttp/application_setup.py 81.9% <ø> (ø)
...e-library/src/servicelib/aiohttp/client_session.py 0.0% <0.0%> (ø)
...rary/src/servicelib/aiohttp/config_schema_utils.py 0.0% <ø> (ø)
...ervice-library/src/servicelib/aiohttp/incidents.py 84.6% <ø> (ø)
...library/src/servicelib/aiohttp/jsonschema_specs.py 0.0% <ø> (ø)
...ry/src/servicelib/aiohttp/jsonschema_validation.py 0.0% <ø> (ø)
...library/src/servicelib/aiohttp/monitor_services.py 0.0% <ø> (ø)
...rvice-library/src/servicelib/aiohttp/monitoring.py 0.0% <ø> (ø)
... and 125 more

@pcrespov pcrespov added the t:maintenance Some planned maintenance work label Aug 31, 2021
@GitHK
Copy link
Contributor

GitHK commented Sep 1, 2021

@pcrespov while trying to make this PR work, it was not possible to do so.

I propose closing this PR.

In alternative I'd create a new library called utilslib containing general utility functions, to be shared among other packages and services:

  • for my future use case there are are 2 functions which need to be shared by servicelib, director-v2, simcore-sdk, dynamic-sidecar.
  • I would also add a test to avoid check that this new library installs other simcore packages (avoids future issues).

FYI: @sanderegg @mguidon

@GitHK
Copy link
Contributor

GitHK commented Sep 1, 2021

After a discussione with @pcrespov, we decided to break dependency between simcore-sdk and servicelib.

  • will copy functions from the apiopg_utils
  • archiving_utils will go into a separate very small focused library called archivinglib.

@GitHK
Copy link
Contributor

GitHK commented Sep 2, 2021

In the end, I've managed to refactor some code and avoid adding additional libraries

@GitHK
Copy link
Contributor

GitHK commented Sep 6, 2021

Think I've managed to address everything.

@pcrespov one more thing does not convince me. In the webserver I did not change anything inside requirements. But I've managed to fix all the tests and to use the "new import paths". I think you mentioned something about the code still being there even after installing servicelib without the aiohttp part. That would explain why it works.

@pcrespov
Copy link
Member Author

pcrespov commented Sep 7, 2021

Think I've managed to address everything.

@pcrespov one more thing does not convince me. In the webserver I did not change anything inside requirements. But I've managed to fix all the tests and to use the "new import paths". I think you mentioned something about the code still being there even after installing servicelib without the aiohttp part. That would explain why it works.

@GitHK is there any reason why you add the _aiohttp.in requirements in the webserver? Most of the dependencies in servicelib[aiohttp] are already included in web-server installer and the test coverage is not 100%.
I did some refactoring and added it

@pcrespov pcrespov force-pushed the maintenance/refactor-servicelib branch from be1b818 to 863ac1d Compare September 7, 2021 19:55
@pcrespov pcrespov marked this pull request as ready for review September 7, 2021 19:55
Copy link
Contributor

@GitHK GitHK left a comment

Choose a reason for hiding this comment

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

👍 I think it looks all good now.

@pcrespov pcrespov merged commit 3502447 into ITISFoundation:master Sep 8, 2021
@pcrespov pcrespov deleted the maintenance/refactor-servicelib branch September 8, 2021 09:47
Copy link
Member

@mguidon mguidon left a comment

Choose a reason for hiding this comment

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

Cleaning Lady was here...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a:services-library issues on packages/service-libs t:maintenance Some planned maintenance work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants