Skip to content

Conversation

@sethvargo
Copy link
Contributor

This updates the Cloud KMS samples to match the other languages (sans Ruby and Python which are still on my list). It uses the new sample format Brent and I discussed on chat.

@sethvargo sethvargo requested a review from bshaffer May 1, 2020 15:15
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label May 1, 2020
@sethvargo sethvargo requested a review from dwsupplee May 1, 2020 15:16
@sethvargo sethvargo force-pushed the sethvargo/kms_samples branch 2 times, most recently from b4db29b to 52cc6e5 Compare May 1, 2020 18:20
) {
// PHP has limited support for asymmetric encryption operations.
// Specifically, openssl_public_encrypt() does not allow customizing
// algorithms or padding. Thus, it is not currently possible to use PHP
Copy link
Contributor

Choose a reason for hiding this comment

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

This is confusing to me... does KMS require custom algorithms?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Talked on chat. I'm open to other words here.

private static function runSampleFile($sampleFile, $sampleName, $params = [])
{
$sampleFile = __DIR__ . sprintf('/../src/%s', $sampleFile);
$sampleName = str_replace('_sample', '', $sampleName) . '_sample';
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the purpose of this? As far as I can tell, _sample is never used in the sample name parameter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This entire function is never called directly (only runSample). It's a lower-level API that someone can call when the name of the file doesn't match the name of the sample.

Copy link
Contributor

@dwsupplee dwsupplee left a comment

Choose a reason for hiding this comment

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

These looks really nice @sethvargo, nice work.

@sethvargo sethvargo force-pushed the sethvargo/kms_samples branch from 6e530cf to cef66da Compare May 5, 2020 02:02
@sethvargo sethvargo requested review from bshaffer and dwsupplee May 5, 2020 02:04
@sethvargo sethvargo force-pushed the sethvargo/kms_samples branch from cef66da to 08e0ede Compare May 5, 2020 02:36
@sethvargo sethvargo force-pushed the sethvargo/kms_samples branch from 08e0ede to 72b93b8 Compare May 5, 2020 02:45
@sethvargo sethvargo merged commit 72c35fd into master May 5, 2020
@sethvargo sethvargo deleted the sethvargo/kms_samples branch May 5, 2020 20:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants