Skip to content

Preparing the modules to be split#100

Merged
jtratner merged 10 commits into
counsyl:v4from
anujkumar93:split_packages_pre
Dec 4, 2018
Merged

Preparing the modules to be split#100
jtratner merged 10 commits into
counsyl:v4from
anujkumar93:split_packages_pre

Conversation

@anujkumar93

@anujkumar93 anujkumar93 commented Dec 4, 2018

Copy link
Copy Markdown
Collaborator

@jtratner

This PR handles getting the complete repo ready for its separation into individual packages of stor, stor_s3, stor_swift, stor_dx.

Background

This is a continuation of the story to split stor across 4 modules : stor, stor_dx, stor_swift, stor_s3. This is done to easy the dependencies that need to be installed to use a particular fact of stor, and to make it more modular. This PR is preceded by #99 for this story.

Changes

  • Splitting the test_posix, test_windows, test_cli, test_utils, and test.py modules into the different packages

  • Changes in relevant docs corresponding to these tests

  • 'is_swift_path', 'is_dx_path', 'is_s3_path' are separated into their individual packages

  • convert_swiftstack and is_writeable changed to have a try catch block on import swiftpath, for checking is_swift_path

Tested via:

The testing suite was run and all the tests that were split still passed after the split.

TODOS

@jtratner jtratner left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

code looks good % comments about hoisting and convert_swiftstack change! I gotta take a second look at test code just to confirm copied over well

Comment thread stor/utils.py
Comment thread stor/utils.py Outdated
Comment thread stor_swift/swift.py
use_manifest=False,
headers=None):
headers=None,
**kwargs):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

what kwargs are getting passed through here?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I introduced kwargs to make the signature of upload uniform across stor_swift, stor_s3 and stor_dx. The kwargs passed here come from copy/copytree from posix to swift. But ofc kwargs isn't used anywhere in this function internally here. Can totally remove this if you want.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

what are the keyword arguments? Can we list them explicitly instead?

@anujkumar93 anujkumar93 Dec 4, 2018

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I mean the explicit keyword arguments are condition, use_manifest, headers. The keyword arguments to swift_retry_options will also end up here, but as I said they aren't utilized internally in the fn. This is just to make it uniform with the fn signature of upload in s3 and dx. In S3, we already had def upload(self, source, condition=None, use_manifest=False, headers=None, **kwargs):

Comment thread stor/cli.py
return swiftstack.s3_to_swift(path)
else:
raise ValueError("invalid path for conversion: '%s'" % path)
try:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
try:
try:
from stor_swift import is_swift_path
from stor_s3 import is_s3_path
except ImportError:
raise ImportError('stor-swift and stor-s3 must be installed to use convert-swiftstack command')

since none of this really makes sense without stor_swift and stor_s3 installed :)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

actually this assumes you need both. We actually need only one of stor_swift or stor_s3 installed for convert_swiftstack. If stor_s3 is installed and swift path is passed, it'll still raise the error. I think there needs to be two try-catch blocks here. Probably.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

For now let's require both, leave the rest of it as the original, and if there's desire in the future to be able to convert without having both installed, we can do that.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Gotcha, changed.

Comment thread stor/utils.py
Comment thread docs/testing.rst Outdated
jtratner and others added 3 commits December 4, 2018 08:13
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.

2 participants