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

Skip 0-sized blobs when listing storage. Fixes #595. #597

Merged
merged 3 commits into from
Jul 20, 2023

Conversation

rzvoncek
Copy link
Contributor

@rzvoncek rzvoncek commented Jul 14, 2023

Fixes #595.

I'm proposing filtering the 0-sized files one step lower than the issue suggests. Doing the filter in list_backups() would need more changes around the codebase, for example in the functions dealing with validating the backup. This way I only had to patch the local and abstract storage list_objects().

@rzvoncek rzvoncek force-pushed the radovan/595-hiearchical-azure branch 2 times, most recently from c84fdea to 14ca1dc Compare July 14, 2023 13:30
@codecov
Copy link

codecov bot commented Jul 14, 2023

Codecov Report

Merging #597 (d951b6e) into master (ec425ec) will decrease coverage by 1.21%.
The diff coverage is 100.00%.

❗ Current head d951b6e differs from pull request most recent head ebf6036. Consider uploading reports for the commit ebf6036 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #597      +/-   ##
==========================================
- Coverage   80.56%   79.35%   -1.21%     
==========================================
  Files          58       58              
  Lines        4558     4601      +43     
  Branches      678      688      +10     
==========================================
- Hits         3672     3651      -21     
- Misses        860      919      +59     
- Partials       26       31       +5     
Impacted Files Coverage Δ
medusa/storage/__init__.py 91.83% <100.00%> (-1.42%) ⬇️
medusa/storage/abstract_storage.py 93.33% <100.00%> (+0.10%) ⬆️
medusa/storage/local_storage.py 96.96% <100.00%> (+0.19%) ⬆️

... and 6 files with indirect coverage changes

@rzvoncek rzvoncek marked this pull request as draft July 17, 2023 09:35
@rzvoncek rzvoncek force-pushed the radovan/595-hiearchical-azure branch from 14ca1dc to 29cbd4e Compare July 17, 2023 09:44
@rzvoncek rzvoncek force-pushed the radovan/595-hiearchical-azure branch from 29cbd4e to d951b6e Compare July 17, 2023 13:12
@rzvoncek rzvoncek marked this pull request as ready for review July 18, 2023 07:53
Copy link
Contributor

@adejanovski adejanovski left a comment

Choose a reason for hiding this comment

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

Just one non blocking suggestion, otherwise LGTM ✅

medusa/storage/abstract_storage.py Outdated Show resolved Hide resolved
@sonarcloud
Copy link

sonarcloud bot commented Jul 19, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

No Coverage information No Coverage information
76.9% 76.9% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

@rzvoncek rzvoncek merged commit d68fea3 into master Jul 20, 2023
27 of 30 checks passed
adejanovski pushed a commit that referenced this pull request Sep 4, 2023
* Skip 0-sized blobs when listing storage. Fixes #595.

* Reading index blobs ignores wrongly pathed ones. Fixes #595.

* Remove unused argument about allowing listing empty files
adejanovski pushed a commit that referenced this pull request Sep 4, 2023
* Skip 0-sized blobs when listing storage. Fixes #595.

* Reading index blobs ignores wrongly pathed ones. Fixes #595.

* Remove unused argument about allowing listing empty files
@rzvoncek rzvoncek deleted the radovan/595-hiearchical-azure branch September 22, 2023 11:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Listing backups fails on some installations against Azure
2 participants