Switch to google-cloud-storage gem, delegate `url(expires:)` to #presign #16

rosskevin · 2017-10-12T19:06:41Z

This is a fairly major change, it might make sense given semver guidelines to bump this to 1.0.0, though I see no reason why backwards compatibility is not maintained.

Motivation:

switch off legacy gem to the current gem google-cloud-storage
get project/credential discovery with new gem
allow url(expires:) to delegate to #presign - matching S3 usage (but with GCS options)

Notes:

Added test/test_env_setup.sh to ease getting started for new contributors
Added contribution notes for both the new script, as well as manual setup
Added :project option for overriding in main class
Added more tests

Fixes #15

All tests are green.

…is specified

renchap · 2017-10-12T19:44:36Z

Thanks 👍 I glanced over it and it looks great. I will do a deeper review later.

Bumping to 0.3.0 is fine regarding to semver, as a major version of 0 means that anything can break I any time. I bump the minor version for changes breaking the API until we reach 1.0.0. And for this I would like to have more eyes / users on the code before declaring it stable (which may come soon, as it looks like this gems is starting to be used 😄)

renchap · 2017-10-12T19:48:27Z

Also it might be worth it to look at #8 and try to get it working with this new implementation.

renchap

Here is my first pass.

As projects are easy to create on GCP, what would you think of creating a new project in your test setup script?
This would allow to namespace everything and not mess up with any already-existing project the user has configured. It also allows to easily cleanup everything by deleting the project.
The script can check if the project, SA and bucket exists and tell the user to either change the project name or delete it before running the script again.
It would also be a good idea to display what the script will do at the start (create project $project, create bucket $bucket, create $sa service account, and ask the user if he is fine for this.

I may be paranoid, but my experience shows that it is a good idea to check everything before executing anything on a cloud console/API

renchap · 2017-10-12T22:01:10Z

.gitignore

@@ -1,2 +1,4 @@
 .env
 pkg/
+.ruby-gemset
+.ruby-version


Missing new line

renchap · 2017-10-12T22:10:58Z

lib/shrine/storage/google_cloud_storage.rb

+              acl: @default_acl
+          ) do |f|
+            # update the additional options
+            @object_options.merge(_options).each_pair do |key, value|


Rename to options. The _ is a convention to indicate the argument is not used.

renchap · 2017-10-12T22:11:29Z

lib/shrine/storage/google_cloud_storage.rb

          )
        end
      end

+      # URL to the remote file, accepts options for customizing the URL
      def url(id, **_options)


Same for _options

renchap · 2017-10-12T22:21:09Z

lib/shrine/storage/google_cloud_storage.rb

-        end
-
+        prefix = "#{@prefix}/" if @prefix
+        all_objects = get_bucket.files prefix: prefix


It will not work if the bucket contains a lot of objects.
See https://googlecloudplatform.github.io/google-cloud-ruby/#/docs/google-cloud/v0.44.0/google/cloud/storage/file/list
It might be a good idea to use next and call batch_delete for each page, compared to use all which could create a very big array.

renchap · 2017-10-12T22:29:42Z

test/gcs_test.rb

+    end
+  end
+
+  describe "#url" do


A test for url() calls without expires could be great here too for completeness

renchap · 2017-10-12T22:35:44Z

test/test_env_setup.sh

+
+# Download the service account json
+mkdir -p $(dirname $GOOGLE_CLOUD_KEYFILE)
+rm -f $GOOGLE_CLOUD_KEYFILE


I would prefer to fail if $GOOGLE_CLOUD_KEYFILE exists rather than deleting it. If it exists and is a private key for a SA, I would not like it to be deleted.

renchap · 2017-10-12T22:36:19Z

test/test_env_setup.sh

+GCS_BUCKET=${GCS_SA}-${GOOGLE_CLOUD_PROJECT}
+gsutil mb -p $GOOGLE_CLOUD_PROJECT gs://$GCS_BUCKET/
+
+rm -f ../.env


You do not need to remove the file if you override it on the next line (using >)

renchap · 2017-10-12T22:43:50Z

test/test_env_setup.sh

+gsutil mb -p $GOOGLE_CLOUD_PROJECT gs://$GCS_BUCKET/
+
+rm -f ../.env
+cat >../.env <<EOL


I would prefer to not use a relative path here, as the script may be invoked from the root of the project, like this: test/test_env_setup.sh.
Getting the script directory is tricky to do properly in shell. I would prefer to output the variables and ask the user to put them in .env. This also gives the user the ability to put them elsewhere if needed.

renchap · 2017-10-12T22:43:59Z

test/test_env_setup.sh

+GCS_BUCKET=${GCS_BUCKET}
+GOOGLE_CLOUD_PROJECT=${GOOGLE_CLOUD_PROJECT}
+GOOGLE_CLOUD_KEYFILE=${GOOGLE_CLOUD_KEYFILE}
+EOL


Missing new line

renchap · 2017-10-12T22:44:49Z

test/test_env_setup.sh

@@ -0,0 +1,39 @@
+#!/usr/bin/env bash


Please rename the script to something clearer, like create_test_environment.sh

rosskevin · 2017-10-12T22:58:47Z

@renchap I'll work on some of these change.

Regarding the test script:
I'm not interested in putting more time into the test script. You are welcome to make all these changes, I just wanted to move the ball forward a bit. This is an improvement on what was there.

Everything you mention is a good idea, and I welcome you or others to take it on. I just have spent my allotted time and need to move on.

I'll get the code review changes done now.

rosskevin · 2017-10-12T23:14:51Z

I have completed all the code review changes above where I added the 👍

The only thing I did not expand on was the test script changes as I mentioned above. I also re-ran the tests - all green.

rosskevin · 2017-10-26T14:59:07Z

@renchap is this going to be merged? I want to get on a release for production instead of a fork/commit hash.

renchap · 2017-10-27T09:29:47Z

I am very busy right now with other stuff, but I will do it ASAP.

renchap · 2017-11-19T14:24:41Z

Thanks again @rosskevin, I just rebased your commits, improved the test environment creation script and fixed a few things. It would be amazing if you could review the changes on master and tell me if everything is good for you!

rosskevin added 5 commits October 12, 2017 14:26

convert client to google-cloud-storage 1.6.0

30c148e

create test setup script and update readme with instructions

054331c

fix options and usage of copy/create_file

3a9af25

all tests pass

640ab54

delegate #presign to gcs, #url will delegate to #presign if :expires …

d255155

…is specified

rosskevin force-pushed the google-cloud-storage branch from 13d7b0b to d255155 Compare October 12, 2017 19:27

fix link in readme

d9edbff

rosskevin mentioned this pull request Oct 12, 2017

Change open to use Down.open #8

Closed

renchap self-requested a review October 12, 2017 21:20

renchap requested changes Oct 12, 2017

View reviewed changes

code review changes

77374ec

renchap closed this Nov 19, 2017

This was referenced Nov 19, 2017

Url with expires #15

Closed

Fix object content_type when moving from objects #17

Closed

Add ability to set acl and object_options per upload #14

Closed

rosskevin deleted the google-cloud-storage branch December 21, 2018 20:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch to google-cloud-storage gem, delegate `url(expires:)` to #presign #16

Switch to google-cloud-storage gem, delegate `url(expires:)` to #presign #16

rosskevin commented Oct 12, 2017

renchap commented Oct 12, 2017

renchap commented Oct 12, 2017

renchap left a comment

renchap Oct 12, 2017

renchap Oct 12, 2017

renchap Oct 12, 2017

renchap Oct 12, 2017

renchap Oct 12, 2017

renchap Oct 12, 2017

renchap Oct 12, 2017

renchap Oct 12, 2017

renchap Oct 12, 2017

renchap Oct 12, 2017

rosskevin commented Oct 12, 2017

rosskevin commented Oct 12, 2017

rosskevin commented Oct 26, 2017

renchap commented Oct 27, 2017

renchap commented Nov 19, 2017

Switch to google-cloud-storage gem, delegate url(expires:) to #presign #16

Switch to google-cloud-storage gem, delegate url(expires:) to #presign #16

Conversation

rosskevin commented Oct 12, 2017

renchap commented Oct 12, 2017

renchap commented Oct 12, 2017

renchap left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rosskevin commented Oct 12, 2017

rosskevin commented Oct 12, 2017

rosskevin commented Oct 26, 2017

renchap commented Oct 27, 2017

renchap commented Nov 19, 2017

Switch to google-cloud-storage gem, delegate `url(expires:)` to #presign #16

Switch to google-cloud-storage gem, delegate `url(expires:)` to #presign #16