-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
feat(storage): add quickstart sample for storage control #2621
Conversation
Here is the summary of changes. You are about to add 1 region tag.
This comment is generated by snippet-bot.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good, but I wonder if we should create a separate folder for Storage control? I think we should.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know whether @amanda-tarafa has different preferences in terms of resource name usage and overloads vs request objects.
StorageControlClient storageControl = StorageControlClient.Create(); | ||
GetStorageLayoutRequest request = new GetStorageLayoutRequest | ||
{ | ||
Name = StorageLayoutName.Format("_", bucketName), // Set project to "_" for global bucket" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than explicitly formatting to a string, I'd set the StorageLayoutName
property instead (if you really need a request object - see below):
GetStorageLayoutRequest request = new GetStorageLayoutRequest
{
StorageLayoutName = new StorageLayoutName("_", bucketName");
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes +1 to this.
{ | ||
Name = StorageLayoutName.Format("_", bucketName), // Set project to "_" for global bucket" | ||
}; | ||
StorageLayout response = storageControl.GetStorageLayout(request); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a relatively long-winded way of doing things. We generate an overload accepting the name:
StorageLayout response = storageControl.GetStorageLayout(new StorageLayoutName("_", bucket));
(By all means separate out the StorageLayoutName initialization to a separate variable.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a sample guideline, or at least there used to be one, about showing the request object overload for most samples. This is meant to be a hint for users that "there's likely more you can do".
I'm usually lenient with whatever approach is used, so I'm fine either way. I agree that it is more legible to use the simpler overload though, if that's enough.
@amanda-tarafa, about your comment regarding a separate folder: I'm basing this off of the other client libraries that have implemented this so far, which have it within the storage samples folder (https://github.com/googleapis/java-storage/pull/2479/files), (https://github.com/GoogleCloudPlatform/golang-samples/pull/3988/files) |
Adds a quickstart sample for storage control