-
Notifications
You must be signed in to change notification settings - Fork 592
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
storage: storage.createBucket('name', cb) #168
Comments
This makes sense to me. Is there any reason we can't do this @silvolu? https://developers.google.com/storage/docs/json_api/v1/buckets/insert |
We should also provide listing, deletion, etc. Adding this to M2. |
That's a good point. What API can we use that makes sense for that? The following suggestion would require creating a var storage = gcloud.storage;
var buckets = new storage.Buckets(/*credentials*/);
// buckets instanceof Buckets
buckets.list(function(err, buckets) {
// buckets = Bucket[]
});
buckets.get('Photos', function(err, photos) {
// photos instanceof Bucket
});
buckets.create('Albums', function(err, albums) {
// albums instanceof Bucket
});
|
I'm not a big fan of It does make sense to specify options only once so I think we should do that. But what about even sooner than Buckets? The credentials themselves should work for all gcloud apis, right? So what about something radical like: var gcloud = require('gcloud');
var client = new gcloud.Client({ credentials: /* ... */ });
// or just gcloud.options({ credentials: /* ... */ });
var storage = client.storage;
// or just gcloud.storage if we use .options() example above
storage.buckets.list(function(err, buckets) {
var photos = buckets.get('Photos');
if(!photos) {
buckets.create('Photos', function(err, photos) {
photos.upload('funnycat.gif', { /* opts */ }, function(err) {
console.log(err || 'Uploaded funny cat gif');
});
});
}
}); What calls need to be async here? |
You won't catch it in any of my stuff, either. I would like to argue that
If we can do this, I like this much better. I'll have to read through this again for a refresher, but #46 (comment) was a blocker.
|
I'm not saying using require('gcloud')
.options({ credentials: require('./key.json') })
.storage
.createBucket('bucket name', callback); |
The more you hate on it, the more I agree with you. It feels like forcing the developer to use
This stuff should probably be talked about in a separate thread (perhaps the one I linked before). For now, we should just talk about how we can implement the Buckets api in our current structure. If and when we can support something like the above example, that will require a whole revamp of our code anyway. * Sorry for the long sentence, but if anyone deserves that, it's you :P (just kidding of course) |
Haha sorry, I ramble sometimes. But I really want this API client to be something we don't even have to teach developers to use. For storage, there's only a few key concepts we need to teach them: auth, buckets, files. Then the documentation will show them all the methods they can run on those concepts i.e. list buckets, upload file, list files, delete bucket, etc.. We can give them a couple examples and they can run with it. I want developers to be able to correctly GUESS how our client works, that's how easy it should be 😄 It's a lot to ask, but I think we can do it. Current example (from our docs): var gcloud = require('gcloud');
var storage = gcloud.storage;
var bucket;
// From Google Compute Engine:
bucket = new storage.Bucket({
bucketName: YOUR_BUCKET_NAME
});
// Or from elsewhere:
bucket = new storage.Bucket({
bucketName: YOUR_BUCKET_NAME,
keyFilename: '/path/to/the/key.json'
});
bucket.write('demo.txt', 'Hello World', function(err) {
console.log(err || 'Created demo.txt');
}); Could be simplified to: var gcloud = require('gcloud');
gcloud.options({ keyFilename: !ON_COMPUTE && '/path/to/the/key.json' });
// some env variable used here to detect if you're on GCE
var storage = gcloud.storage;
storage.bucket('ryans-notes').write('demo.txt', 'Hello World', function(err) {
console.log(err || 'Created demo.txt');
}); |
As an aside, if we made these changes, it would start to look and act a lot more like This library has the added benefit that we can hand-craft the api structure instead of having to tweak a bunch of templates and push it through a code generator (which is a huge pain the ass and requires a lot design upfront). I'm trying to bring those "design upfront" ideas here because it helps save a lot of time but we can still iterate and tweak as needed with more flexibility. (That was a long aside) 😳 |
I see this as two separate discussions. Part 1 being allowing creation of a connected gcloud object, and Part 2 being how we can expand the Storage api we offer. I think it would make sense to continue Part 1 in the place where it left off: #46 and we can put Part 2 on hold if you think Part 1 needs to be resolved first.
I really like the simplicity here. This way behaves as it does currently, as far as waits for the call to // assuming `stephen-notes` doesn't exist...
var bucket = storage.bucket('stephen-notes');
['things', 'to', 'write', 'to', 'my', 'secret', 'journal'].forEach(function(note) {
bucket.write(note, function(err) {
// 7 api calls and fails.
});
});
// vs..
storage.bucket('stephen-notes', function(err, bucket) {
if (err) {
// don't waste time thinking bucket operations will work
return;
}
// make api calls that will work.
}); Any thoughts on all that? Should we allow assuredness while getting a bucket that it exists, or is it ok to continue to wait until the API calls? As a note, when we allow creation of buckets, what method name should we use? |
Opened #191 for
Are you saying that
Can we support both? If you supply a callback, make the request and callback a success/fail. Don't supply a callback, then assume it exists and create the bucket only locally. We can also supply methods to check if a bucket exists / create a bucket so they can do the callback step themselves. Then storage.bucket(name) is just way to define what bucket we want to perform actions on, and yes we will receive errors if the bucket doesn't exist or they don't have permission. storage.bucket('stephen-notes').exists(function(nope) {
if(nope) {
storage.bucket('stephen-notes').create(function(err, created) {
storage.bucket('stephen-notes').upload(filename, callback);
});
} else {
storage.bucket('stephen-notes').upload(filename, callback);
}
}); |
Yeah, to verify the bucket exists, and prevent future api calls to fail. It's going to fail eventually, might as well fail when the user tries to access a bucket is my thought, so they can address the issue before making possibly multiple failure calls to
No, I said we don't do it currently. Just asking your opinion on if you think it's important. As far as |
To be less harsh on And just another note on it, since I just noticed this, |
Now that I think about it, I really prefer using |
Yes.
That does make sense. I would vote to remove the Can you now tackle how you see working with "buckets" methods, like listing, creating, and deleting? |
Hmm... Let's go back to first year and recall that calling a method on something is delegating it to perform some action. Delegation. So, storage is responsible for creating a bucket. So then we are left with So we could do: storage.createBucket(name);
// or
var bucket = storage.bucket('name');
storage.create(bucket, cb); So now my question is should we do: var bucket = storage.bucket('name');
bucket.upload('filename', cb);
// or
// blah, I don't like this one.
storage.upload('bucketName', 'filename', cb)
// or
var bucket = storage.bucket('name');
var file = bucket.file('filename');
bucket.upload(file, cb); |
(I'm really indecisive). Lemme sit on this and come up with something more concrete taking into consideration the different layers of storage. |
No problem. var bucket = storage.bucket('name');
bucket.upload('filename', cb); I think operations to a bucket should happen on the storage level, and operations on a bucket should happen on the bucket level, basically matching how the API is broken apart: https://developers.google.com/storage/docs/json_api/v1. The storage module, to me, needs to look like this: storage = {
bucket: function() {},
createBucket: function(name, callback) {
apiCreateBucket(name, function() {
if (err) { callback(err); return; }
// return a bucket instance to the createBucket callback
callback(null, storage.bucket(name));
});
},
deleteBucket: function(callback) {},
// also could be listBuckets, can't remember which we've used in the past
getBuckets: function(callback) {}
};
storage.bucket('new-bucket').create(); I know the one liner is nice, but it's backwards. "Get -> Create", should be "Create -> Get". storage.bucket('my-bucket').delete(function(err) {}); would be confused with storage.bucket('my-bucket').remove('', function(err) {}); The first deleting a bucket from the bucket instance, and the second deleting a file in the bucket. Better to keep bucket removal clear, and on the storage level. Those are just my thoughts! |
One last concept for your review, which is going back to my first example from an earlier comment: var storage = gcloud.storage;
var buckets = new storage.Buckets(/*credentials*/);
// buckets instanceof Buckets
buckets.list(function(err, buckets) {
// buckets = Bucket[]
});
buckets.get('Photos', function(err, photos) {
// photos instanceof Bucket
});
buckets.create('Albums', function(err, albums) {
// albums instanceof Bucket
}); I liked this because of the hierarchical logic. You create/add a bucket to a collection of buckets, and you get a bucket from a collection of buckets. Using the advancements we've made in the rest of the thread, with some small tweaks, this doesn't look too bad to me, and maintains that hierarchy. var buckets = gcloud.storage.buckets;
buckets.list(function(err, results) {
// results = Bucket[]
});
var PhotosBucket = buckets.get('Photos');
// PhotosBucket instanceof Bucket
buckets.create('Albums', function(err, albums) {
// albums instanceof Bucket
// New delete method example:
buckets.delete('Albums', function(err) {});
}); So, using your earlier use case of: var bucket = storage.bucket('name');
bucket.upload('filename', cb); This would look like: var bucket = storage.buckets.get('name');
bucket.upload('filename', cb); Which is a loss in terms of character count, but I think a plus in terms of understandability when you consider we need to share the |
Refactored discussion: #194 :) |
* test: use fully qualified request type name in tests PiperOrigin-RevId: 475685359 Source-Link: googleapis/googleapis@7a12973 Source-Link: googleapis/googleapis-gen@370c729 Copy-Tag: eyJwIjoiLmdpdGh1Yi8uT3dsQm90LnlhbWwiLCJoIjoiMzcwYzcyOWUyYmEwNjJhMTY3NDQ5YzI3ODgyYmE1ZjM3OWM1YzM0ZCJ9 * 🦉 Updates from OwlBot post-processor See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md * 🦉 Updates from OwlBot post-processor See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
- [ ] Regenerate this pull request now. PiperOrigin-RevId: 470911839 Source-Link: googleapis/googleapis@3527566 Source-Link: googleapis/googleapis-gen@f16a1d2 Copy-Tag: eyJwIjoiLmdpdGh1Yi8uT3dsQm90LnlhbWwiLCJoIjoiZjE2YTFkMjI0ZjAwYTYzMGVhNDNkNmE5YTFhMzFmNTY2ZjQ1Y2RlYSJ9 feat: accept google-gax instance as a parameter Please see the documentation of the client constructor for details. PiperOrigin-RevId: 470332808 Source-Link: googleapis/googleapis@d4a2367 Source-Link: googleapis/googleapis-gen@e97a1ac Copy-Tag: eyJwIjoiLmdpdGh1Yi8uT3dsQm90LnlhbWwiLCJoIjoiZTk3YTFhYzIwNGVhZDRmZTczNDFmOTFlNzJkYjdjNmFjNjAxNjM0MSJ9
Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com> Co-authored-by: Justin Beckwith <justin.beckwith@gmail.com>
This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [uuid](https://togithub.com/uuidjs/uuid) | devDependencies | major | [`^7.0.0` -> `^8.0.0`](https://renovatebot.com/diffs/npm/uuid/7.0.3/8.0.0) | --- ### Release Notes <details> <summary>uuidjs/uuid</summary> ### [`v8.0.0`](https://togithub.com/uuidjs/uuid/blob/master/CHANGELOG.md#​800-httpsgithubcomuuidjsuuidcomparev703v800-2020-04-29) [Compare Source](https://togithub.com/uuidjs/uuid/compare/v7.0.3...v8.0.0) ##### ⚠ BREAKING CHANGES - For native ECMAScript Module (ESM) usage in Node.js only named exports are exposed, there is no more default export. ```diff -import uuid from 'uuid'; -console.log(uuid.v4()); // -> 'cd6c3b08-0adc-4f4b-a6ef-36087a1c9869' +import { v4 as uuidv4 } from 'uuid'; +uuidv4(); // ⇨ '9b1deb4d-3b7d-4bad-9bdd-2b0d7b3dcb6d' ``` - Deep requiring specific algorithms of this library like `require('uuid/v4')`, which has been deprecated in `uuid@7`, is no longer supported. Instead use the named exports that this module exports. For ECMAScript Modules (ESM): ```diff -import uuidv4 from 'uuid/v4'; +import { v4 as uuidv4 } from 'uuid'; uuidv4(); ``` For CommonJS: ```diff -const uuidv4 = require('uuid/v4'); +const { v4: uuidv4 } = require('uuid'); uuidv4(); ``` ##### Features - native Node.js ES Modules (wrapper approach) ([#​423](https://togithub.com/uuidjs/uuid/issues/423)) ([2d9f590](https://togithub.com/uuidjs/uuid/commit/2d9f590ad9701d692625c07ed62f0a0f91227991)), closes [#​245](https://togithub.com/uuidjs/uuid/issues/245) [#​419](https://togithub.com/uuidjs/uuid/issues/419) [#​342](https://togithub.com/uuidjs/uuid/issues/342) - remove deep requires ([#​426](https://togithub.com/uuidjs/uuid/issues/426)) ([daf72b8](https://togithub.com/uuidjs/uuid/commit/daf72b84ceb20272a81bb5fbddb05dd95922cbba)) ##### Bug Fixes - add CommonJS syntax example to README quickstart section ([#​417](https://togithub.com/uuidjs/uuid/issues/417)) ([e0ec840](https://togithub.com/uuidjs/uuid/commit/e0ec8402c7ad44b7ef0453036c612f5db513fda0)) ##### [7.0.3](https://togithub.com/uuidjs/uuid/compare/v7.0.2...v7.0.3) (2020-03-31) ##### Bug Fixes - make deep require deprecation warning work in browsers ([#​409](https://togithub.com/uuidjs/uuid/issues/409)) ([4b71107](https://togithub.com/uuidjs/uuid/commit/4b71107d8c0d2ef56861ede6403fc9dc35a1e6bf)), closes [#​408](https://togithub.com/uuidjs/uuid/issues/408) ##### [7.0.2](https://togithub.com/uuidjs/uuid/compare/v7.0.1...v7.0.2) (2020-03-04) ##### Bug Fixes - make access to msCrypto consistent ([#​393](https://togithub.com/uuidjs/uuid/issues/393)) ([8bf2a20](https://togithub.com/uuidjs/uuid/commit/8bf2a20f3565df743da7215eebdbada9d2df118c)) - simplify link in deprecation warning ([#​391](https://togithub.com/uuidjs/uuid/issues/391)) ([bb2c8e4](https://togithub.com/uuidjs/uuid/commit/bb2c8e4e9f4c5f9c1eaaf3ea59710c633cd90cb7)) - update links to match content in readme ([#​386](https://togithub.com/uuidjs/uuid/issues/386)) ([44f2f86](https://togithub.com/uuidjs/uuid/commit/44f2f86e9d2bbf14ee5f0f00f72a3db1292666d4)) ##### [7.0.1](https://togithub.com/uuidjs/uuid/compare/v7.0.0...v7.0.1) (2020-02-25) ##### Bug Fixes - clean up esm builds for node and browser ([#​383](https://togithub.com/uuidjs/uuid/issues/383)) ([59e6a49](https://togithub.com/uuidjs/uuid/commit/59e6a49e7ce7b3e8fb0f3ee52b9daae72af467dc)) - provide browser versions independent from module system ([#​380](https://togithub.com/uuidjs/uuid/issues/380)) ([4344a22](https://togithub.com/uuidjs/uuid/commit/4344a22e7aed33be8627eeaaf05360f256a21753)), closes [#​378](https://togithub.com/uuidjs/uuid/issues/378) </details> --- ### Renovate configuration :date: **Schedule**: "after 9am and before 3pm" (UTC). :vertical_traffic_light: **Automerge**: Disabled by config. Please merge this manually once you are satisfied. :recycle: **Rebasing**: Whenever PR is behind base branch, or you tick the rebase/retry checkbox. :no_bell: **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [WhiteSource Renovate](https://renovate.whitesourcesoftware.com). View repository job log [here](https://app.renovatebot.com/dashboard#googleapis/nodejs-recaptcha-enterprise).
Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
This PR was generated using Autosynth. 🌈 Synth log will be available here: https://source.cloud.google.com/results/invocations/041f5df7-f5d3-4b2a-9ede-0752bf41c185/targets
Our README says that we must create a bucket on the developers console before we can use it in this library. Can't we provide a method to allow developers to create buckets on the fly?
The text was updated successfully, but these errors were encountered: