S3 versioned cloning #176
S3 versioned cloning #176
Conversation
Codecov Report
@@ Coverage Diff @@
## master #176 +/- ##
=========================================
+ Coverage 82.31% 82.62% +0.3%
=========================================
Files 10 10
Lines 509 518 +9
=========================================
+ Hits 419 428 +9
Misses 90 90 |
docs/source/bookstore_api.yaml
Outdated
| @@ -9,7 +9,7 @@ info: | |||
| license: | |||
| name: BSD 3-clause | |||
| url: https://github.com/nteract/bookstore/blob/master/LICENSE | |||
| version: 2.3.0.dev0 | |||
| version: 2.5.1 | |||
There was a problem hiding this comment.
Is this the correct version bump for this?
There was a problem hiding this comment.
I was bumping it to match what the version of bookstore itself would be upon release, minus the dev0.
We don't have a separate versioning scheme for the API (we can create one if we think that's best). I removed the dev0 simply because that when we release this, it would match the published version of the package.
I'm open to other suggestions as to how to handle this, though :)
There was a problem hiding this comment.
Might be good to have the version kept in sync between the two files programmatically when running the bdist_wheel. If that's a pain to do, then we should instead just add it explicitly to the RELEASING.md instructions imo.
There was a problem hiding this comment.
I think we could set up bumpversion do handle that, but I think for now the low overhead way of making it part of the RELEASING.md makes sense. I think there are a couple of other places where that would help too.
There was a problem hiding this comment.
I realize I'm coming in with a fresh take on this... The version in this YAML file could be different from the library itself since it's the REST API. 🤷♂
bookstore/clone.py
Outdated
| template_params = { | ||
| "post_model": model, | ||
| "clone_api_url": clone_api_url, | ||
| "redirect_contents_url": redirect_contents_url, | ||
| "source_description": f"'{s3_object_key}' from the s3 bucket '{s3_bucket}'", | ||
| "source_description": f"'{s3_object_key}'{' version: '+s3_version_id if s3_version_id else ''} from the s3 bucket '{s3_bucket}'", |
There was a problem hiding this comment.
Might be better to move the conditional version text into a variable above to make it more readable
There was a problem hiding this comment.
this change has been made in the latest version of this PR
docs/source/bookstore_api.yaml
Outdated
| @@ -9,7 +9,7 @@ info: | |||
| license: | |||
| name: BSD 3-clause | |||
| url: https://github.com/nteract/bookstore/blob/master/LICENSE | |||
| version: 2.3.0.dev0 | |||
| version: 2.5.1 | |||
There was a problem hiding this comment.
Might be good to have the version kept in sync between the two files programmatically when running the bdist_wheel. If that's a pain to do, then we should instead just add it explicitly to the RELEASING.md instructions imo.
mpacer
force-pushed
the
s3_versioned_cloning
branch
from
c0b7223 to
95109e0
Compare
- also, matches API version to current bookstore install version
mpacer
force-pushed
the
s3_versioned_cloning
branch
from
95109e0 to
9e286ae
Compare
|
@MSeal @captainsafia I just rebased this PR on master and pushed updates to address the version concern & the string legibility concern. I created #177 so a bumpversion setup can be pursued in a separate PR. |
|
Since it looks like feedback is addressed and there's another thread for the one point of contention, I'll go ahead and merge this. |
This adds the ability to clone an old version from a versioned s3 bucket.
Addresses #172
Unfortunately we cannot test for this functionality in our minio based integration tests. This is because minio does not support versioned buckets:
see minio/minio#6494 for the latest efforts to implement ending in a decision to not do so and minio/minio#1309 for some of the original discussion around it.
This pulls out functionality around creating the kwargs to send to S3 so that we can test that logic independently.
This also updates the API docs.
@MSeal @captainsafia I added you as reviewers per your request earlier today at the team meeting.