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

Refactoring storage: removed s3wrapper #2295

Merged
merged 28 commits into from
Apr 27, 2021

Conversation

pcrespov
Copy link
Member

@pcrespov pcrespov commented Apr 26, 2021

What do these changes do?

REMOVES packages/s3wrapper module because :

  • it was only used in storage service,
  • was not well maintained and
  • added a lot of unnecessary dependencies to other many services

Highlights

  • Moved as a submodule of storage service. (simcore_service_storage.s3wrapper)
  • Removes a unnecessary dependencies in webserver, sidecar and simcore-sdk services
  • CHANGES pytest_simcore.minio_services to avoid dependency with s3wrapper
  • ADDS pytest_simcore.monkeypatch_extra to enhance testing
  • FIXES missing constraints in simcore_sdk/requirements/_test.in

NOTE: constraint minio<7 due to breaking API changes -> needs review of entire python client.

Related issue/s

This is a part of PR #1588 separated in a single PR to simplify review.

How to test

Most tests in repo affected

Checklist

@pcrespov pcrespov changed the title Refactor/remove s3wrapper WIP: Refactor/remove s3wrapper Apr 26, 2021
@codecov
Copy link

codecov bot commented Apr 26, 2021

Codecov Report

Merging #2295 (b6950c0) into master (ae5b0f2) will decrease coverage by 0.0%.
The diff coverage is 100.0%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #2295     +/-   ##
========================================
- Coverage    71.8%   71.8%   -0.1%     
========================================
  Files         504     505      +1     
  Lines       19702   19856    +154     
  Branches     1931    1948     +17     
========================================
+ Hits        14161   14260     +99     
- Misses       5069    5120     +51     
- Partials      472     476      +4     
Flag Coverage Δ
integrationtests 62.0% <ø> (-0.1%) ⬇️
unittests 67.1% <100.0%> (+<0.1%) ⬆️

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

Impacted Files Coverage Δ
...ervices/storage/src/simcore_service_storage/dsm.py 72.3% <100.0%> (ø)
services/storage/src/simcore_service_storage/s3.py 89.1% <100.0%> (ø)
...src/simcore_service_storage/s3wrapper/s3_client.py 67.5% <100.0%> (ø)
...webserver/computation_comp_tasks_listening_task.py 84.9% <0.0%> (-3.3%) ⬇️
...ce_webserver/resource_manager/garbage_collector.py 74.8% <0.0%> (-1.0%) ⬇️

@pcrespov pcrespov self-assigned this Apr 26, 2021
@pcrespov pcrespov added this to the Schwarznasenschaf milestone Apr 26, 2021
@pcrespov pcrespov force-pushed the refactor/remove-s3wrapper branch from e39eae6 to 08c3b0d Compare April 26, 2021 16:24
@pcrespov pcrespov force-pushed the refactor/remove-s3wrapper branch from 08c3b0d to 54907b6 Compare April 26, 2021 16:28
@pcrespov pcrespov marked this pull request as ready for review April 26, 2021 18:37
@pcrespov pcrespov changed the title WIP: Refactor/remove s3wrapper Refactoring storage: removed s3wrapper Apr 26, 2021
@pcrespov pcrespov requested review from sanderegg, GitHK and mguidon April 26, 2021 18:37
@pcrespov pcrespov added a:storage issue related to storage service t:maintenance Some planned maintenance work labels Apr 26, 2021
Copy link
Member

@sanderegg sanderegg left a comment

Choose a reason for hiding this comment

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

always good to remove stuff!

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.

One less dependency to handle, nice!

@pcrespov pcrespov merged commit f133e8d into ITISFoundation:master Apr 27, 2021
@pcrespov pcrespov deleted the refactor/remove-s3wrapper branch April 27, 2021 08:57
@pcrespov pcrespov mentioned this pull request Apr 27, 2021
14 tasks
@pcrespov pcrespov mentioned this pull request Jun 21, 2021
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a:storage issue related to storage service t:maintenance Some planned maintenance work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants