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

fix: dandi-etag carries extra component beyond md5 #24

Merged
merged 3 commits into from
May 28, 2021
Merged

Conversation

satra
Copy link
Member

@satra satra commented May 28, 2021

The check for digest assumed an md5sum. however, the digest has an extra component and are of the form: <md5sum>-<chunks>

@codecov
Copy link

codecov bot commented May 28, 2021

Codecov Report

Merging #24 (9d134de) into master (a65afe2) will increase coverage by 0.57%.
The diff coverage is 97.29%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #24      +/-   ##
==========================================
+ Coverage   93.93%   94.50%   +0.57%     
==========================================
  Files          11       11              
  Lines         824      856      +32     
==========================================
+ Hits          774      809      +35     
+ Misses         50       47       -3     
Flag Coverage Δ
unittests 94.50% <97.29%> (+0.57%) ⬆️

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

Impacted Files Coverage Δ
dandischema/models.py 91.83% <90.00%> (+0.88%) ⬆️
dandischema/tests/test_metadata.py 100.00% <100.00%> (ø)
dandischema/tests/test_models.py 94.64% <100.00%> (+1.61%) ⬆️

Continue to review full report at Codecov.

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

@satra satra added patch Increment the patch version when merged release Create a release when this pr is merged labels May 28, 2021
@satra satra requested a review from yarikoptic May 28, 2021 17:45
except (KeyError, ValueError):
raise ValueError("Digest must have both valid dandi-etag and sha2-256.")
digest = values[DigestType.dandi_etag]
if "-" not in digest or len(digest.split("-")[0]) != 32:
Copy link
Member

Choose a reason for hiding this comment

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

I think

Suggested change
if "-" not in digest or len(digest.split("-")[0]) != 32:
if len(digest.split("-")[0]) != 32:

would be sufficient but ok either way

@yarikoptic
Copy link
Member

Left minor non-consequential comment which might be wrong, so let's proceed

@yarikoptic yarikoptic merged commit 4570f14 into master May 28, 2021
@yarikoptic yarikoptic deleted the fix/digest branch May 28, 2021 18:36
@yarikoptic yarikoptic removed the release Create a release when this pr is merged label May 28, 2021
@yarikoptic yarikoptic mentioned this pull request May 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch Increment the patch version when merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants