Skip to content

Conversation

hiranya911
Copy link
Contributor

@hiranya911 hiranya911 commented Jun 30, 2017

Some code borrowed from #38

Exposes Google Cloud Storage API via the Java Admin SDK:

  • Take a dependency on GCS Node.js client (and the type definitions from DefinitelyTyped)
  • Introducing Storage type that wraps the GCS storage client
  • Expose the GCS buckets via a new bucket() method.

go/firebase-admin-gcs

Copy link
Contributor

@bojeil-google bojeil-google left a comment

Choose a reason for hiding this comment

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

Looks really good. Just a few nits and some minor suggestions.

*/
public delete(): Promise<void> {
// There are no resources to clean up.
return Promise.resolve(undefined);
Copy link
Contributor

Choose a reason for hiding this comment

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

Promise.resolve()
Otherwise this would return Promise

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

} else {
bucketName = this.appInternal.options.storageBucket;
}
if (typeof bucketName !== 'string' || bucketName === '') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: you can also use validator.sNonEmptyString

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


function testDefaultBucket() {
const bucket = admin.storage().bucket();
return verifyBucket(bucket, 'storage().bucker()')
Copy link
Contributor

Choose a reason for hiding this comment

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

bucket

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

import {FirebaseApp} from '../../../src/firebase-app';
import {Storage} from '../../../src/storage/storage';

describe('Messaging', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Storage

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


describe('bucket(valid)', () => {
it('should return a bucket object', () => {
expect(storage.bucket('foo').name).to.equal('foo');
Copy link
Contributor

Choose a reason for hiding this comment

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

Should you add another valid test when no bucket name is added?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@hiranya911
Copy link
Contributor Author

Thanks for the feedback. I've made all the suggested changes.

@hiranya911 hiranya911 merged commit 1f3d34a into master Aug 11, 2017
@hiranya911 hiranya911 deleted the hkj-gcs-api branch August 11, 2017 17:23
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