From 45a775f6a978afe03a3c2fc557df360ae07fe88a Mon Sep 17 00:00:00 2001 From: M Pacer Date: Mon, 2 Dec 2019 13:42:08 -0800 Subject: [PATCH 1/6] Add s3 version cloning & update tests --- bookstore/clone.py | 25 +++++++++++++++++-------- bookstore/tests/test_clone.py | 5 ++--- 2 files changed, 19 insertions(+), 11 deletions(-) diff --git a/bookstore/clone.py b/bookstore/clone.py index f6fe46e..71efe89 100644 --- a/bookstore/clone.py +++ b/bookstore/clone.py @@ -82,7 +82,7 @@ class BookstoreCloneHandler(IPythonHandler): Helper to access bookstore settings. get(self) Checks for valid storage settings and render a UI for clone options. - construct_template_params(self, s3_bucket, s3_object_key) + construct_template_params(self, s3_bucket, s3_object_key, s3_version_id=None) Helper to populate Jinja template for cloning option page. get_template(self, name) Loads a Jinja template and its related settings. @@ -115,13 +115,15 @@ async def get(self): if s3_object_key == '' or s3_object_key == '/': raise web.HTTPError(400, "Requires an S3 object key in order to clone") + s3_version_id = self.get_argument("s3_version_id", default=None) + self.log.info(f"Setting up cloning landing page for {s3_object_key}") - template_params = self.construct_template_params(s3_bucket, s3_object_key) + template_params = self.construct_template_params(s3_bucket, s3_object_key, s3_version_id) self.set_header('Content-Type', 'text/html') self.write(self.render_template('clone.html', **template_params)) - def construct_template_params(self, s3_bucket, s3_object_key): + def construct_template_params(self, s3_bucket, s3_object_key, s3_version_id=None): """Helper that takes valid S3 parameters and populates UI template Returns @@ -137,12 +139,12 @@ def construct_template_params(self, s3_bucket, s3_object_key): else: redirect_contents_url = url_path_join(self.base_url, self.default_url) clone_api_url = url_path_join(self.base_url, "/api/bookstore/clone") - model = {"s3_bucket": s3_bucket, "s3_key": s3_object_key} + model = {"s3_bucket": s3_bucket, "s3_key": s3_object_key, "s3_version_id": s3_version_id} 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}'", } return template_params @@ -179,7 +181,7 @@ def initialize(self): self.session = aiobotocore.get_session() - async def _clone(self, s3_bucket, s3_object_key): + async def _clone(self, s3_bucket, s3_object_key, s3_version_id=None): """Main function that handles communicating with S3 to initiate the clone. Parameters @@ -188,6 +190,8 @@ async def _clone(self, s3_bucket, s3_object_key): Log (usually from the NotebookApp) for logging endpoint changes. s3_object_key: str The the path we wish to clone to. + s3_version_id: str, optional + The version id provided. Default is None, which gets the latest version """ self.log.info(f"bucket: {s3_bucket}") @@ -202,7 +206,10 @@ async def _clone(self, s3_bucket, s3_object_key): ) as client: self.log.info(f"Processing clone of {s3_object_key}") try: - obj = await client.get_object(Bucket=s3_bucket, Key=s3_object_key) + s3_kwargs = {"Bucket": s3_bucket, "Key": s3_object_key} + if s3_version_id is not None: + s3_kwargs['VersionId'] = s3_version_id + obj = await client.get_object(**s3_kwargs) content = (await obj['Body'].read()).decode('utf-8') except ClientError as e: status_code = e.response['ResponseMetadata'].get('HTTPStatusCode') @@ -224,6 +231,7 @@ async def post(self): "s3_bucket": string, "s3_key": string, "target_path"?: string + "s3_version_id"?: string } The response payload should match the standard Jupyter contents @@ -242,9 +250,10 @@ async def post(self): target_path = model.get("target_path", "") or os.path.basename( os.path.relpath(s3_object_key) ) + s3_version_id = model.get("s3_version_id", None) self.log.info(f"About to clone from {s3_object_key}") - obj, content = await self._clone(s3_bucket, s3_object_key) + obj, content = await self._clone(s3_bucket, s3_object_key, s3_version_id=s3_version_id) content_model = self.build_content_model(content, target_path) diff --git a/bookstore/tests/test_clone.py b/bookstore/tests/test_clone.py index 6f588e7..ca1b061 100644 --- a/bookstore/tests/test_clone.py +++ b/bookstore/tests/test_clone.py @@ -109,9 +109,8 @@ async def test_get_success(self): await success_handler.get() def test_gen_template_params(self): - success_handler = self.get_handler('/bookstore/clone?s3_bucket=hello&s3_key=my_key') expected = { - 'post_model': {'s3_bucket': 'hello', 's3_key': 'my_key'}, + 'post_model': {'s3_bucket': 'hello', 's3_key': 'my_key', 's3_version_id': None}, 'clone_api_url': '/api/bookstore/clone', 'redirect_contents_url': '/', 'source_description': "'my_key' from the s3 bucket 'hello'", @@ -124,7 +123,7 @@ def test_gen_template_params(self): def test_gen_template_params_base_url(self): expected = { - 'post_model': {'s3_bucket': 'hello', 's3_key': 'my_key'}, + 'post_model': {'s3_bucket': 'hello', 's3_key': 'my_key', 's3_version_id': None}, 'clone_api_url': '/my_base_url/api/bookstore/clone', 'redirect_contents_url': '/my_base_url', 'source_description': "'my_key' from the s3 bucket 'hello'", From 6536315a828e94ed896c3e8e85bbeaefed912f02 Mon Sep 17 00:00:00 2001 From: M Pacer Date: Mon, 2 Dec 2019 13:42:35 -0800 Subject: [PATCH 2/6] Update docs to describe s3_version_id parameters & model values --- docs/source/bookstore_api.yaml | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/docs/source/bookstore_api.yaml b/docs/source/bookstore_api.yaml index e9fe5ac..9ec053c 100644 --- a/docs/source/bookstore_api.yaml +++ b/docs/source/bookstore_api.yaml @@ -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 externalDocs: description: Find out more about Bookstore url: https://bookstore.readthedocs.io/en/latest/ @@ -47,6 +47,13 @@ paths: style: form schema: type: string + - name: s3_version_id + in: query + description: S3 object key being requested + required: false + style: form + schema: + type: string responses: 200: description: successful operation @@ -171,6 +178,8 @@ components: type: string s3_key: type: string + s3_version_id: + type: string target_path: type: string FSCloneFileRequest: From 2bb7f83797e9a5da574d768ab261fdc93cc66a8d Mon Sep 17 00:00:00 2001 From: M Pacer Date: Mon, 2 Dec 2019 13:42:57 -0800 Subject: [PATCH 3/6] Add new test for s3_version_id functionality --- bookstore/tests/test_clone.py | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/bookstore/tests/test_clone.py b/bookstore/tests/test_clone.py index ca1b061..ccf7df8 100644 --- a/bookstore/tests/test_clone.py +++ b/bookstore/tests/test_clone.py @@ -121,6 +121,19 @@ def test_gen_template_params(self): ) assert expected == output + def test_gen_template_params_s3_version_id(self): + expected = { + 'post_model': {'s3_bucket': 'hello', 's3_key': 'my_key', 's3_version_id': "my_version"}, + 'clone_api_url': '/api/bookstore/clone', + 'redirect_contents_url': '/', + 'source_description': "'my_key' version: my_version from the s3 bucket 'hello'", + } + success_handler = self.get_handler('/bookstore/clone?s3_bucket=hello&s3_key=my_key&s3_version_id=my_version') + output = success_handler.construct_template_params( + s3_bucket="hello", s3_object_key="my_key", s3_version_id="my_version" + ) + assert expected == output + def test_gen_template_params_base_url(self): expected = { 'post_model': {'s3_bucket': 'hello', 's3_key': 'my_key', 's3_version_id': None}, From 609fef6cab2924c7da0b02aed66060b2787d4f9b Mon Sep 17 00:00:00 2001 From: M Pacer Date: Mon, 2 Dec 2019 13:57:51 -0800 Subject: [PATCH 4/6] Isolate utility inside BookstoreCloneAPIHandler, add tests for utility --- bookstore/clone.py | 21 ++++++++++++++++++--- bookstore/tests/test_clone.py | 31 ++++++++++++++++++++++++++++++- 2 files changed, 48 insertions(+), 4 deletions(-) diff --git a/bookstore/clone.py b/bookstore/clone.py index 71efe89..99b5206 100644 --- a/bookstore/clone.py +++ b/bookstore/clone.py @@ -181,6 +181,23 @@ def initialize(self): self.session = aiobotocore.get_session() + def _build_s3_request_object(self, s3_bucket, s3_object_key, s3_version_id=None): + """Helper to build object request with the appropriate keys for S3 APIs. + + Parameters + ---------- + s3_bucket: str + Log (usually from the NotebookApp) for logging endpoint changes. + s3_object_key: str + The the path we wish to clone to. + s3_version_id: str, optional + The version id provided. Default is None, which gets the latest version + """ + s3_kwargs = {"Bucket": s3_bucket, "Key": s3_object_key} + if s3_version_id is not None: + s3_kwargs['VersionId'] = s3_version_id + return s3_kwargs + async def _clone(self, s3_bucket, s3_object_key, s3_version_id=None): """Main function that handles communicating with S3 to initiate the clone. @@ -206,9 +223,7 @@ async def _clone(self, s3_bucket, s3_object_key, s3_version_id=None): ) as client: self.log.info(f"Processing clone of {s3_object_key}") try: - s3_kwargs = {"Bucket": s3_bucket, "Key": s3_object_key} - if s3_version_id is not None: - s3_kwargs['VersionId'] = s3_version_id + s3_kwargs = self._build_s3_request_object(s3_bucket, s3_object_key, s3_version_id) obj = await client.get_object(**s3_kwargs) content = (await obj['Body'].read()).decode('utf-8') except ClientError as e: diff --git a/bookstore/tests/test_clone.py b/bookstore/tests/test_clone.py index ccf7df8..7711970 100644 --- a/bookstore/tests/test_clone.py +++ b/bookstore/tests/test_clone.py @@ -128,7 +128,9 @@ def test_gen_template_params_s3_version_id(self): 'redirect_contents_url': '/', 'source_description': "'my_key' version: my_version from the s3 bucket 'hello'", } - success_handler = self.get_handler('/bookstore/clone?s3_bucket=hello&s3_key=my_key&s3_version_id=my_version') + success_handler = self.get_handler( + '/bookstore/clone?s3_bucket=hello&s3_key=my_key&s3_version_id=my_version' + ) output = success_handler.construct_template_params( s3_bucket="hello", s3_object_key="my_key", s3_version_id="my_version" ) @@ -225,6 +227,33 @@ async def test_private_clone_nonsense_params(self): with pytest.raises(HTTPError): await success_handler._clone(s3_bucket, s3_object_key) + def test_build_s3_request_object(self): + expected = {"Bucket": "my_bucket", "Key": "my_key"} + s3_bucket = "my_bucket" + s3_object_key = "my_key" + post_body_dict = {"s3_key": "my_key", "s3_bucket": "my_bucket"} + success_handler = self.post_handler(post_body_dict) + actual = success_handler._build_s3_request_object(s3_bucket, s3_object_key) + assert actual == expected + + def test_build_s3_request_object_version_id(self): + expected = {"Bucket": "my_bucket", "Key": "my_key", "VersionId": "my_version"} + + s3_bucket = "my_bucket" + s3_object_key = "my_key" + s3_version_id = "my_version" + + post_body_dict = { + "s3_key": s3_object_key, + "s3_bucket": s3_bucket, + "s3_version_id": s3_version_id, + } + success_handler = self.post_handler(post_body_dict) + actual = success_handler._build_s3_request_object( + s3_bucket, s3_object_key, s3_version_id=s3_version_id + ) + assert actual == expected + def test_build_post_response_model(self): content = "some arbitrary content" expected = { From d4a537f1ce611a0ed8d7746938b6edf246cd6b61 Mon Sep 17 00:00:00 2001 From: M Pacer Date: Fri, 6 Dec 2019 12:01:16 -0800 Subject: [PATCH 5/6] Move additional version text for response page into explicit variable Per review. --- bookstore/clone.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/bookstore/clone.py b/bookstore/clone.py index 99b5206..84761e9 100644 --- a/bookstore/clone.py +++ b/bookstore/clone.py @@ -140,11 +140,12 @@ def construct_template_params(self, s3_bucket, s3_object_key, s3_version_id=None redirect_contents_url = url_path_join(self.base_url, self.default_url) clone_api_url = url_path_join(self.base_url, "/api/bookstore/clone") model = {"s3_bucket": s3_bucket, "s3_key": s3_object_key, "s3_version_id": s3_version_id} + version_text = ' version: ' + s3_version_id if s3_version_id else '' template_params = { "post_model": model, "clone_api_url": clone_api_url, "redirect_contents_url": redirect_contents_url, - "source_description": f"'{s3_object_key}'{' version: '+s3_version_id if s3_version_id else ''} from the s3 bucket '{s3_bucket}'", + "source_description": f"'{s3_object_key}'{version_text} from the s3 bucket '{s3_bucket}'", } return template_params From 9e286ae9adcd92f83638a1661b960d31e5cc4d30 Mon Sep 17 00:00:00 2001 From: M Pacer Date: Fri, 6 Dec 2019 13:23:38 -0800 Subject: [PATCH 6/6] Add instructions for everywhere versions need updates to RELEASING.md - also, matches API version to current bookstore install version --- RELEASING.md | 7 +++++-- docs/source/bookstore_api.yaml | 2 +- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/RELEASING.md b/RELEASING.md index 38ee16b..6cb9797 100644 --- a/RELEASING.md +++ b/RELEASING.md @@ -14,7 +14,10 @@ version is used. ## Create the release -- [ ] Update version number `bookstore/_version.py` +- [ ] Update version numbers in + - [ ] `bookstore/_version.py` (version_info) + - [ ] `docs/source/conf.py` (version and release) + - [ ] `docs/source/bookstore_api.yaml` (info.version) - [ ] Commit the updated version - [ ] Clean the repo of all non-tracked files: `git clean -xdfi` - [ ] Commit and tag the release @@ -48,4 +51,4 @@ twine upload dist/* - [ ] If all went well: - Change `bookstore/_version.py` back to `.dev` - - Push directly to `master` and push `--tags` too. \ No newline at end of file + - Push directly to `master` and push `--tags` too. diff --git a/docs/source/bookstore_api.yaml b/docs/source/bookstore_api.yaml index 9ec053c..b7cbe70 100644 --- a/docs/source/bookstore_api.yaml +++ b/docs/source/bookstore_api.yaml @@ -9,7 +9,7 @@ info: license: name: BSD 3-clause url: https://github.com/nteract/bookstore/blob/master/LICENSE - version: 2.5.1 + version: 2.5.1dev0 externalDocs: description: Find out more about Bookstore url: https://bookstore.readthedocs.io/en/latest/