-
Notifications
You must be signed in to change notification settings - Fork 598
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: add storage class enums #787
Conversation
Thanks for doing this! Even though I wanted all lowercase, I'm not sure that was a good call. Constants always want to be loud, and we already have Another thing, ACLs are available for buckets and files, so it made sense to have them sit atop both at the Storage class level. With Example: var gcloud = require('gcloud');
var gcs = gcloud.gcs({ projectId: 'grape-spaceship-123' });
gcs.createBucket({
storageClass: gcs.bucket.storageClass.DRA
}, function(err, bucket, apiResponse) {}); I really don't prefer this constant system over just a property on the metadata object: var gcloud = require('gcloud');
var gcs = gcloud.gcs({ projectId: 'grape-spaceship-123' });
gcs.createBucket({
dra: true
}, function(err, bucket, apiResponse) {}); Or best-guessing from var gcloud = require('gcloud');
var gcs = gcloud.gcs({ projectId: 'grape-spaceship-123' });
gcs.createBucket({
storageClass: 'dra' // => we expand this to `DURABLE_REDUCED_AVAILABILITY`
}, function(err, bucket, apiResponse) {}); @jgeewax which approach do you think is best here? |
So I ended up just uppercasing the constant names to gcs.createBucket({
storageClass: gcs.DRA
}, callback); |
I strongly think we should go with the second suggestion above. That's the easiest solution for the user, and won't inflate our API. These constants only affect createBucket and can only be used there. |
I think we could easily support both? If we use the metadata property version we'll still need to have an alias to the expanded version. What are your feelings on something like Storage.DRA = 'DURABLE_REDUCED_AVAILABILITY';
Storage.prototype.createBucket = function(name, metadata, callback) {
var body = extend(metadata, { name: name });
['dra', 'nearline'].forEach(function(storageClass) {
if (body[storageClass]) {
delete body[storageClass];
body.storageClass = Storage[storageClass.toUpperCase()];
}
});
this.makeReq_('POST, '', null, body, callback);
}; |
I still think it's overkill. We can keep the expanded version in a map local to the createBucket function. This is a very small use case, I don't think we should stuff random metadata properties on the "global" Storage class. |
@stephenplusplus Ok, I changed it to metadata properties. I updated the documentation to show they are optional properties and added links to the official doc for each. I also added a couple of unit tests for each. Would you prefer reduce over forEach? |
Yay! Can you just add "@"resources for https://cloud.google.com/storage/docs/storage-classes, Nearline, and DRA? |
I think how you have it is good. I would have probably just done: if (body.dra) {
body.storageClass = 'DUR...';
delete body.dra;
}
if (body.nearline) {
body.storageClass = 'NEARLINE';
delete body.nearline;
} So any fanciness you have is good with me :) |
done! |
uppercased constants changed constants to metadata properties added resource links
storage: add storage class enums
Thanks! |
Committer: @cherba PiperOrigin-RevId: 389755489
🤖 I have created a release \*beep\* \*boop\* --- ## [4.6.0](https://www.github.com/googleapis/nodejs-speech/compare/v4.5.6...v4.6.0) (2021-08-10) ### Features * add total_billed_time response field ([#787](https://www.github.com/googleapis/nodejs-speech/issues/787)) ([171cba0](https://www.github.com/googleapis/nodejs-speech/commit/171cba0bb8f7bd12ab96f296824e0acf7a0698d7)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Committer: @cherba PiperOrigin-RevId: 389755489
🤖 I have created a release \*beep\* \*boop\* --- ## [4.6.0](https://www.github.com/googleapis/nodejs-speech/compare/v4.5.6...v4.6.0) (2021-08-10) ### Features * add total_billed_time response field ([#787](https://www.github.com/googleapis/nodejs-speech/issues/787)) ([171cba0](https://www.github.com/googleapis/nodejs-speech/commit/171cba0bb8f7bd12ab96f296824e0acf7a0698d7)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Closes #779
Implemented per conversation in original issue, however it might be better to group these together for documentation purposes. The current implementation creates links for both
dra
andnearline
, where I imagine it would be better to lump them together into a single property to consolidate the documentation on storage classes.e.g.