Skip to content
This repository was archived by the owner on Apr 8, 2024. It is now read-only.

Feature/gcp support - #10

Open
orenshk wants to merge 6 commits into
rstojnic:masterfrom
orenshk:feature/gcp-support
Open

Feature/gcp support#10
orenshk wants to merge 6 commits into
rstojnic:masterfrom
orenshk:feature/gcp-support

Conversation

@orenshk

@orenshk orenshk commented Sep 18, 2018

Copy link
Copy Markdown

Support for storing files in Google Cloud Storage.

  • Added GCPRemoteStorage class that implements RemoteStorage
  • Added support in lazydata CLI for adding a remote, but not for creating credentials. I feel this is job best left done to Google's client, but I'm happy to discuss further.

@rstojnic

Copy link
Copy Markdown
Owner

Hey, thanks very much for this - looks very good!

Re: setting up credentials. Agreed, I've only added it for AWS because it was easy to do, but I appreciate that there are more steps than simply providing the keys.

Will review in more details in the evening and hoping to merge soon.

@rstojnic rstojnic left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

The tests seem to be failing - could you have a look, thanks!

Comment thread requirements.txt
boto3>=1.8.6
lazy-import>=0.2.2
google-api-python-client>=1.7.4
google-cloud-storage>=1.12.0

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

These dependencies should be added to setup.py as well, so they are installed together with the library when installing with pip.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

oh but isn't that why you were using lazy_module? Because then having these in setup.py will force non-gcp users to install the client as well.

I can add an extras_require to setup.py, so users can do
pip install lazydata["gcp"] to install the gcp requirements.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

The primary reason for lazy_import was because boto3 has a really long load time, so that adds to startup time when using the lazydata CLI.

I've just looked at these two libraries - the two of them with all the dependencies are about 4MB to download. So, wondering how much hassle it is to have to install them additionally vs having to download them on first install? What do you think?

@orenshk

orenshk commented Sep 18, 2018

Copy link
Copy Markdown
Author

oops, forgot to update circleci, will make another commit.

@orenshk

orenshk commented Sep 19, 2018

Copy link
Copy Markdown
Author

Hey @rstojnic, to fix circleci tests I'll need to pass some google cloud related environment variables to the build, including a secret. Let me know how you want me to pass them to you.

made changes to PR to highlight the env variables needed

@rstojnic

Copy link
Copy Markdown
Owner

Hey @rstojnic, to fix circleci tests I'll need to pass some google cloud related environment variables to the build, including a secret. Let me know how you want me to pass them to you.

I'm not sure if I'm comfortable with taking your credentials! I think there is a way of passing them securely into circleci, but I'm worried they might get leaked out accidentally at some point.

My suggestion would be to maybe somehow mark these tests as requiring extra setup so we can exclude them from the circleci run. One idea is to use the @pytest.mark.skipif decorator to detect a CircleCI run through an environment variable, but open to other suggestions!

I'll run all tests locally on my machine with my credentials when I do releases to make sure nothing is broken for a release. I'll have to do something similar for AWS anyways.

@orenshk

orenshk commented Sep 19, 2018

Copy link
Copy Markdown
Author

Fair enough, I created credentials specifically for this project that only have access to a single bucket, but I agree that it's a little unnerving. I'll mark them as skipif instead.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants