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

BF/ENH: pass through already versioned s3:// urls while requesting them being versioned #3842

Merged
merged 3 commits into from Oct 30, 2019

Conversation

yarikoptic
Copy link
Member

@yarikoptic yarikoptic commented Oct 29, 2019

Closes #3840

Todos

  • apply to all aws URLs (edit: already covered)

yarikoptic and others added 3 commits Oct 29, 2019
left the other case as NotImplemented although it is not clear yet why
code below could not just be used
@codecov
Copy link

@codecov codecov bot commented Oct 29, 2019

Codecov Report

Merging #3842 into 0.11.x will decrease coverage by 20.75%.
The diff coverage is 0%.

Impacted file tree graph

@@             Coverage Diff             @@
##           0.11.x    #3842       +/-   ##
===========================================
- Coverage   77.17%   56.41%   -20.76%     
===========================================
  Files         256       94      -162     
  Lines       33962    15119    -18843     
===========================================
- Hits        26210     8530    -17680     
+ Misses       7752     6589     -1163
Impacted Files Coverage Δ
datalad/plugin/addurls.py 17.69% <ø> (-0.57%) ⬇️
datalad/support/s3.py 23.36% <0%> (+6.76%) ⬆️
datalad/metadata/metadata.py 22.65% <0%> (-65.75%) ⬇️
datalad/plugin/wtf.py 19.57% <0%> (-58.73%) ⬇️
datalad/customremotes/base.py 26.77% <0%> (-51.62%) ⬇️
datalad/metadata/aggregate.py 14.49% <0%> (-50.89%) ⬇️
datalad/support/sshrun.py 40% <0%> (-50%) ⬇️
datalad/support/cookies.py 35.71% <0%> (-48.58%) ⬇️
datalad/interface/run.py 24.37% <0%> (-46.27%) ⬇️
datalad/support/globbedpaths.py 21.25% <0%> (-43.75%) ⬇️
... and 219 more

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 8eefa55...ce54be6. Read the comment docs.

kyleam
kyleam approved these changes Oct 29, 2019
@kyleam
Copy link
Contributor

@kyleam kyleam commented Oct 29, 2019

The appveyor failure is an unrelated codecov connection failure.

if url_rec.query and 'versionId=' in url_rec.query:
was_versioned = True
all_versions.append(url)
Copy link
Contributor

@kyleam kyleam Oct 29, 2019

Choose a reason for hiding this comment

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

Taking another look, shouldn't this part be done for all URLs, not just ones with an s3 scheme?

Copy link
Member Author

@yarikoptic yarikoptic Oct 29, 2019

Choose a reason for hiding this comment

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

Good point! Indeed it should for all aws URLs. I will refactor tomorrow unless someone beats me to it by then

Copy link
Contributor

@kyleam kyleam Oct 30, 2019

Choose a reason for hiding this comment

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

I looked into this further. We already dealt with this in 58f3d92 (BF: s3: Don't duplicate ID when getting versioned URL, 2018-04-05, #2384), so we should be covered for other URLs. Sorry for the noise.

Copy link
Member Author

@yarikoptic yarikoptic Oct 30, 2019

Choose a reason for hiding this comment

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

Oh, but it does point to inadequacy in comparison to http URLs (in the long run since s3 URLs support officially not implemented) when we request all versions?
Something to eat least add a comment about I think

Copy link
Contributor

@kyleam kyleam Oct 30, 2019

Choose a reason for hiding this comment

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

but it does point to inadequacy in comparison to http URLs (in the long run since s3 URLs support officially not implemented) when we request all versions?

I'm not seeing what the inadequacy is, so I'll leave that comment to you.

Copy link
Member Author

@yarikoptic yarikoptic Oct 30, 2019

Choose a reason for hiding this comment

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

if I understood 58f3d92 correctly, if I provide a versioned http url, and request all versions -- it will get decomposed and I get all versions. With current quick workaround I did, if I provide versioned s3 url, and request all versions -- only that version would be returned.

Copy link
Contributor

@kyleam kyleam Oct 30, 2019

Choose a reason for hiding this comment

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

OK, so you're talking about the same thing that you comment on in the first commit of this PR: the PR doesn't properly retrieve versions for s3:// urls, but we'll pass through a s3:// that already has a version tag. Given that you said the initial question in this thread "pointed to inadequacy", I assumed you must have been referring to something else.

@kyleam kyleam merged commit ce54be6 into datalad:0.11.x Oct 30, 2019
2 checks passed
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.

None yet

2 participants