-
Notifications
You must be signed in to change notification settings - Fork 443
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: introduce StorageControl #7245
Conversation
Here is the summary of changes. You are about to add 10 region tags.
This comment is generated by snippet-bot.
|
*/ | ||
|
||
return [ | ||
'interfaces' => [], |
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.
By it being empty does this mean REST is not available?
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. This is where the http annotations would go. It being empty means someone trying to run this with the rest transport would get a PHP fatal error.
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 something we should be able to fix without too much trouble (see googleapis/gapic-generator-php#705)
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 do not see a major issue into having an error thrown in case REST is specified. The library user does get an actionable insight in that case, so I think we could merge this PR and iterate with gapic-generators later.
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 agree! I was just hoping to release with it because it's a better experience. If we don't get it in by tomorrow, I will still merge this for the release.
Adding "Do Not Merge" label because I'd like to get in googleapis/gapic-generator-php#705 before we release this (although it isn't a blocker) |
'defaultScopes' => self::$serviceScopes, | ||
], | ||
'transportConfig' => [ | ||
'rest' => [ |
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 mentions rest though.
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.
You're right! Thank you for pointing this out. We should be able to remove this in googleapis/gapic-generator-php#707
No description provided.