You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
complete Google Cloud storage implementation (and tests) #14
@mziccard assigning this one to you, if you don't mind.
Lets make sure we get a good coverage for both API and SPI layer.
Do you mind to explore the possibility of a fake/local-service (similar to what we have for the Datastore)?
I had a look around and there seem to be no fake/local-service for Storage as the one we use for the Datastore. In the appengine-gcs-client they save files locally when the application in run in the development server. But they do that at the library level, so no emulator is used.
In gcloud-node they use the actual Cloud Storage to run system tests. They require two environment variables to be set (GCLOUD_TESTS_PROJECT_ID and GCLOUD_TESTS_KEY) and run tests against that project.
Using production GCS is probably OK for integration tests that we can make Travis run (until gcloud emulators provides support for GCS.
Are you OK with taking this task - adding an integration test[s] and completing the unit-tests (with mocks when necessary)?
@mziccard as part of this issue effort can you also go over the storage related open issues and lets
try to resolve some of them?
Issue #109 - My preference, which I feel consistent with the spirit of the JDK and guava, is that anything that can be serializable (especially plain data models/containers) should be as such, but when not a of plain data models (such as BlobInfo, BucketInfo,...) I don't feel strongly about it.
Issue #107 - My preference to wait for Java 8, but I don't mind to change the signUrl signature to accept
also a TimeUnit if you think that would be better that just showing it in the javadoc as we do now.
Issue #62 - I am not in the camp of providing a "key" like reference to Blobs, as I didn't see a true need for this in this API (as oppose to Datastore which we have one) but I don't feel as strongly about it as I feel against using a true URI (as originally proposed). In any case, because this may significantly effect
the API is labeled as "Needed for public announcement".
Issue #60 - This is also a big one and therefore marked as "Needed for public announcement". We need
to see if the bug in the apiary client was fixed and even with or without the fix if we really want the update
to use patch and make sure documentation is clear.
Also: There was a proposal to change signUrl to return a URI instead of a URL but I didn't see a dedicated issue for it. and some of the //todo comments in the code could be removed now (e.g. in StorageImpl "todo: configure timeouts"...
Sure! I started going through the issues and //todos,
On the signURL/signURI matter it was discussed in this PR #93. Seems that using URI was preferred but I agree with you that this functionality is know everywhere (docs, stackoverflow, Amazon S3 docs and API) as getting a signed URL. BTW, Amazon S3 uses URL:
URL s = s3client.generatePresignedUrl(generatePresignedUrlRequest);
@mziccard so what else is pending before we can close this issue?
I consider these as dependencies: #106 - I think we decided not to do anything about it? #291 - Is about to be resolved. #306 #193 - I actually don't see that as a true blocker for this issue, however I think it would be very nice
(maybe we just need to add exists to the RPC layer?)
I think blockers are only: #106 I would do nothing as I am not convinced about the alternatives #291 We are about to close it with #294 #306 As soon as #294 gets merged I'll send a PR for this
I don't think #193 should be a blocker. I would add support for selected fields to all gets (through vararg of field enums?) rather than adding exists to the RPC layers (it seems to me as a more complete solution)
Activity
aozarov commentedon Sep 23, 2015
@mziccard assigning this one to you, if you don't mind.
Lets make sure we get a good coverage for both API and SPI layer.
Do you mind to explore the possibility of a fake/local-service (similar to what we have for the Datastore)?
mziccard commentedon Sep 24, 2015
I had a look around and there seem to be no fake/local-service for Storage as the one we use for the Datastore. In the appengine-gcs-client they save files locally when the application in run in the development server. But they do that at the library level, so no emulator is used.
In gcloud-node they use the actual Cloud Storage to run system tests. They require two environment variables to be set (GCLOUD_TESTS_PROJECT_ID and GCLOUD_TESTS_KEY) and run tests against that project.
aozarov commentedon Sep 24, 2015
Using production GCS is probably OK for integration tests that we can make Travis run (until gcloud emulators provides support for GCS.
Are you OK with taking this task - adding an integration test[s] and completing the unit-tests (with mocks when necessary)?
mziccard commentedon Sep 24, 2015
Yes sure I already started :)
aozarov commentedon Oct 10, 2015
@mziccard as part of this issue effort can you also go over the storage related open issues and lets
try to resolve some of them?
Issue #109 - My preference, which I feel consistent with the spirit of the JDK and guava, is that anything that can be serializable (especially plain data models/containers) should be as such, but when not a of plain data models (such as BlobInfo, BucketInfo,...) I don't feel strongly about it.
Issue #107 - My preference to wait for Java 8, but I don't mind to change the signUrl signature to accept
also a TimeUnit if you think that would be better that just showing it in the javadoc as we do now.
Issue #62 - I am not in the camp of providing a "key" like reference to Blobs, as I didn't see a true need for this in this API (as oppose to Datastore which we have one) but I don't feel as strongly about it as I feel against using a true URI (as originally proposed). In any case, because this may significantly effect
the API is labeled as "Needed for public announcement".
Issue #60 - This is also a big one and therefore marked as "Needed for public announcement". We need
to see if the bug in the apiary client was fixed and even with or without the fix if we really want the update
to use patch and make sure documentation is clear.
Also: There was a proposal to change signUrl to return a URI instead of a URL but I didn't see a dedicated issue for it. and some of the //todo comments in the code could be removed now (e.g. in StorageImpl "todo: configure timeouts"...
mziccard commentedon Oct 11, 2015
Sure! I started going through the issues and //todos,
On the signURL/signURI matter it was discussed in this PR #93. Seems that using
URIwas preferred but I agree with you that this functionality is know everywhere (docs, stackoverflow, Amazon S3 docs and API) as getting a signed URL. BTW, Amazon S3 uses URL:aozarov commentedon Oct 12, 2015
I am fine with keeping it as is then.
aozarov commentedon Oct 29, 2015
@mziccard so what else is pending before we can close this issue?
I consider these as dependencies:
#106 - I think we decided not to do anything about it?
#291 - Is about to be resolved.
#306
#193 - I actually don't see that as a true blocker for this issue, however I think it would be very nice
(maybe we just need to add exists to the RPC layer?)
Anything else?
mziccard commentedon Oct 29, 2015
I think blockers are only:
#106 I would do nothing as I am not convinced about the alternatives
#291 We are about to close it with #294
#306 As soon as #294 gets merged I'll send a PR for this
I don't think #193 should be a blocker. I would add support for selected fields to all gets (through vararg of field enums?) rather than adding exists to the RPC layers (it seems to me as a more complete solution)
aozarov commentedon Oct 29, 2015
Great, so only #306 is a blocker for this issue and your approach for fixing the non-blocker #193 issue sounds good to me.
aozarov commentedon Nov 30, 2015
Closing this issue as all associated issues are closed.
Merge pull request googleapis#14 from garrettjonesgoogle/master
chore(deps): update dependency io.grpc:grpc-bom to v1.25.0 (googleapi…
chore(main): release 0.1.1 (#14)
chore: Generated snippets should not be owned by samples reviewers (#…
chore(deps): update dependency com.google.cloud:google-cloud-shared-c…
chore: add synth.py file (#14)
chore(deps): update dependency com.google.cloud:google-cloud-shared-c…
chore(deps): update dependency junit:junit to v4.13 (googleapis#14)
deps: update dependency com.google.protobuf:protobuf-java to v3.11.1 (#…
deps: update dependency com.google.protobuf:protobuf-java to v3.11.1 (#…
chore(deps): update dependency com.google.api.grpc:proto-google-commo…