RDISCROWD-8392: deprecate old boto. use boto3 only (Updated) #1075
Conversation
Pull Request Test Coverage Report for Build 19482063855Details
💛 - Coveralls |
peterkle
left a comment
peterkle
left a comment
There was a problem hiding this comment.
The core changes look good overall. I just noticed some leftover comments from removed tests that should be cleaned up. Also, the drop in test coverage should be addressed before merging.
| t = task_repo.get_task(task.id) | ||
| expected_url = 'https://s3.storage.com:443/test_bucket/%s/%s/%s/hello.txt' % (project.id, task.id, project.owner.id) | ||
| assert task.gold_answers['test__upload_url'] == expected_url | ||
| # test_task_gold_with_files_in_form removed - obsolete boto implementation test |
There was a problem hiding this comment.
I don't think this comment is necessary because the test is completely deleted.
| bucket = conn.get_bucket('test_bucket', validate=False) | ||
| key = bucket.get_key('test_key', validate=False) | ||
| assert key.generate_url(0).split('?')[0] == 'https://s3.test.com:443/test/test_bucket/test_key' | ||
| # TestS3Connection class removed - all tests were obsolete boto2 implementation tests |
There was a problem hiding this comment.
This comment should be removed as well. It references non-existent tests.
| info={'field': 'value'} | ||
| ) | ||
| datajson = json.dumps(data) | ||
| # test_taskrun_with_upload_json removed - obsolete boto implementation test |
There was a problem hiding this comment.
All of these comments should be removed. They reference non-existent tests.
| content = key.get_contents_as_string() | ||
| duration = perf_counter() - begin_time | ||
| file_path = f"{s3_bucket}/{path}" | ||
| app.logger.info("get_content_and_key_from_s3. Load file contents %s duration %f seconds", file_path, duration) |
There was a problem hiding this comment.
I'm curious why these performance metrics were removed, although we didn't necessarily need them in the first place.
| def get_path(self, path='/', *args, **kwargs): | ||
| ret = super(CustomConnection, self).get_path(path, *args, **kwargs) | ||
| return self.host_suffix + ret | ||
| super().__init__() # super(CustomConnection, self).__init__(*args, **kwargs) |
There was a problem hiding this comment.
The comment seems left here by mistake?
Uh oh!
There was an error while loading. Please reload this page.