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

First Bigtable gapic gen for php #731

Merged
merged 24 commits into from
Nov 17, 2017
Merged

Conversation

andreamlin
Copy link
Contributor

Copied over from api-client-staging

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Nov 8, 2017
@andreamlin
Copy link
Contributor Author

andreamlin commented Nov 9, 2017

PTAL
running php style fixer..

@michaelbausor
Copy link
Contributor

GAPIC code looks good, but there are some other changes that we will need in addition. Check out this PR, which adds the DLP API for the first time: #569

In particular, we will need:

  • Updates to settings files in docs folder
  • LICENSE file
  • README files
  • composer.json file
  • Version bump for proto-client library to a version that includes the newly merged Bigtable protobuf classes

@dwsupplee
Copy link
Contributor

@andreamlin, Did you run artman using PHP 5.5 by any chance? @vam-google discovered an issue where php-cs-fixer was failing silently during generation as it was requiring a version of PHP greater than 5.5.

googleapis/artman#310 &
googleapis/artman#316

should address that issue and will use a ruleset which fixes items like extra whitespace and unused imports.

@andreamlin
Copy link
Contributor Author

andreamlin commented Nov 9, 2017

@dwsupplee I'm running php v5.5.9. That should be fine, right?
I'm also using a later version of artman that contains those changes.

@dwsupplee
Copy link
Contributor

dwsupplee commented Nov 9, 2017

Yeah, v5.5.9 should work!

Do you know which version of php-cs-fixer you are running? It should be v2.2.7 to ensure compatability with your version of PHP. The reason I ask is because it appears the task may have failed:

https://github.com/GoogleCloudPlatform/google-cloud-php/pull/731/files#diff-5235c551bb78376c8ebd0495ed33870aR55 (unused import, should be removed)

https://github.com/GoogleCloudPlatform/google-cloud-php/pull/731/files#diff-5235c551bb78376c8ebd0495ed33870aR222 (extra whitespace)

…-cs-fixer:2.2.7; ran phpcbf style fixer on result
@andreamlin
Copy link
Contributor Author

I was using the wrong version of php-cs-fixer :/
Regen'd using php-cs-fixer 2.7
PTAL

@dwsupplee
Copy link
Contributor

The autogen code LGTM now. Nice work!

We still will need the changes @michaelbausor mentioned earlier in the thread, as well as a VERSION file (it can just be versioned as master right now).

@andreamlin
Copy link
Contributor Author

Added VERSION, license, readme, bigtable/composer.json, and docs files; updated top-level composer.json.
PTAL

@michaelbausor
Copy link
Contributor

LGTM, @dwsupplee and/or @jdpedrie can you also take a look?

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.

We need a readme (ex: https://github.com/GoogleCloudPlatform/google-cloud-php/blob/master/src/Dlp/V2beta1/README.md) in each directory that as a *ServiceClient. Outside of these last few nits, looks good to me.

{
"id": "cloud-bigtable",
"name": "google/cloud-bigtable",
"defaultService": "bigtable/bigtableclient",

This comment was marked as spam.

This comment was marked as spam.

@andreamlin
Copy link
Contributor Author

@dwsupplee Added a README.md to the bigtable/v2 directory.

"pattern": "bigtable\/\\w{1,}",
"services": [{
"title": "BigtableClient",
"type": "bigtable/bigtableclient"

This comment was marked as spam.

This comment was marked as spam.

"id": "cloud-bigtable",
"target": "GoogleCloudPlatform/google-cloud-php-bigtable.git",
"path": "src/Bigtable",
"entry": "BigtableClient.php"

This comment was marked as spam.

This comment was marked as spam.

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.

LGTM. Thanks @andreamlin

@dwsupplee dwsupplee merged commit 1820c01 into googleapis:master Nov 17, 2017
@andreamlin andreamlin deleted the php_bigtable branch November 17, 2017 21:41
@jdpedrie jdpedrie added the api: bigtable Issues related to the Bigtable API. label Dec 22, 2017
@jdpedrie jdpedrie mentioned this pull request Dec 22, 2017
@googleapis googleapis deleted a comment from andreamlin Dec 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: bigtable Issues related to the Bigtable API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants