Skip to content
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: [Logging] update client libraries to support Database operations #4921

Closed

Conversation

gcf-owl-bot[bot]
Copy link
Contributor

@gcf-owl-bot gcf-owl-bot bot commented Jan 5, 2022

  • Regenerate this pull request now.

PiperOrigin-RevId: 419710013

Source-Link: googleapis/googleapis@b7c9d05

Source-Link: https://github.com/googleapis/googleapis-gen/commit/ae498279c4e71cd4aa6e0655e92a693df97472c4
Copy-Tag: eyJwIjoiTG9nZ2luZy8uT3dsQm90LnlhbWwiLCJoIjoiYWU0OTgyNzljNGU3MWNkNGFhNmUwNjU1ZTkyYTY5M2RmOTc0NzJjNCJ9

PiperOrigin-RevId: 419710013

Source-Link: googleapis/googleapis@b7c9d05

Source-Link: googleapis/googleapis-gen@ae49827
Copy-Tag: eyJwIjoiTG9nZ2luZy8uT3dsQm90LnlhbWwiLCJoIjoiYWU0OTgyNzljNGU3MWNkNGFhNmUwNjU1ZTkyYTY5M2RmOTc0NzJjNCJ9
@gcf-owl-bot gcf-owl-bot bot requested review from a team as code owners January 5, 2022 08:23
@trusted-contributions-gcf trusted-contributions-gcf bot added kokoro:force-run Add this label to force Kokoro to re-run the tests. owlbot:run Add this label to trigger the Owlbot post processor. labels Jan 5, 2022
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Jan 5, 2022
@trusted-contributions-gcf trusted-contributions-gcf bot added the owlbot:run Add this label to trigger the Owlbot post processor. label Jan 5, 2022
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Jan 5, 2022
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 5, 2022
@bshaffer bshaffer changed the title feat: update client libraries to support Database operations feat: [Logging] update client libraries to support Database operations Jan 8, 2022
Copy link
Contributor

@bshaffer bshaffer left a comment

Choose a reason for hiding this comment

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

We have removal of static methods and the moving of changes from optional to required.

The latter implies a BC-breaking change in the protobuf layer, and there's nothing we can do about that as far as I know, as that implies a breaking change on the API side.

We could consider fixing this in the Client Library in order to preserve BC, as the API will already be throwing API Exceptions here anyway, and there's no reason to additionally break the user's code.

Logging/src/V2/Gapic/ConfigServiceV2GapicClient.php Outdated Show resolved Hide resolved
*/
public static function billingSinkName($billingAccount, $sink)
Copy link
Contributor

Choose a reason for hiding this comment

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

Removing this method breaks BC

Logging/src/V2/Gapic/ConfigServiceV2GapicClient.php Outdated Show resolved Hide resolved
*/
public static function sinkName($project, $sink)
Copy link
Contributor

Choose a reason for hiding this comment

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

Removing this method breaks BC

*
* @throws ApiException if the remote call fails
*/
public function getCmekSettings($name, array $optionalArgs = [])
Copy link
Contributor

Choose a reason for hiding this comment

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

This new method signature breaks BC. $name moved from an optional argument to a required argument. The previous method signature looked like this:

public function getCmekSettings(array $optionalArgs = [])

This shouldn't be allowed by the API, so we need to look into how this happened.

*/
public function updateExclusion($name, $exclusion, $updateMask, array $optionalArgs = [])
public function updateCmekSettings($name, $cmekSettings, array $optionalArgs = [])
Copy link
Contributor

Choose a reason for hiding this comment

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

This method had both optional parameters $name and $cmekSettings moved to be required arguments.

@bshaffer
Copy link
Contributor

superceded by #5046

@bshaffer bshaffer closed this Jan 24, 2022
@bshaffer bshaffer deleted the owl-bot-9b63199d-de4d-4042-a5a8-9960ce48b0dd branch January 24, 2022 18:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants