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

Introduce auto-generated Gapic client for monitoring API #1657

Merged
merged 6 commits into from
Oct 18, 2016

Conversation

landrito
Copy link
Contributor

@landrito landrito commented Oct 1, 2016

No description provided.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Oct 1, 2016
Copy link
Contributor

@stephenplusplus stephenplusplus left a comment

Choose a reason for hiding this comment

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

Looks awesome, thank you! 👍

@@ -0,0 +1,201 @@
Apache License

This comment was marked as spam.

This comment was marked as spam.

{
"url": "https://github.com/googleapis/googleapis",
"name": "@google-cloud/monitoring",
"version": "0.7.1",

This comment was marked as spam.

This comment was marked as spam.

@@ -0,0 +1,21 @@
{
"url": "https://github.com/googleapis/googleapis",

This comment was marked as spam.

This comment was marked as spam.

"description": "Stackdriver Monitoring library for Node.js",
"main": "src/index.js",
"files": [
"src"

This comment was marked as spam.

This comment was marked as spam.

@@ -0,0 +1,45 @@
Stackdriver Monitoring API for Node.js

This comment was marked as spam.

This comment was marked as spam.

"name": "@google-cloud/monitoring",
"version": "0.7.1",
"author": "Google Inc.",
"description": "Stackdriver Monitoring library for Node.js",

This comment was marked as spam.

"engines": {
"node": ">=0.12.0"
}
}

This comment was marked as spam.

This comment was marked as spam.

"google-proto-files": "^0.8.2",
"google-gax": "^0.7.0",
"extend": "^3.0.0",
"lodash.union": "^4.6.0"

This comment was marked as spam.

This comment was marked as spam.

@landrito
Copy link
Contributor Author

landrito commented Oct 3, 2016

Thanks for the review! This should be ready for another pass.

@stephenplusplus
Copy link
Contributor

Thanks! There are some linting errors: https://travis-ci.org/GoogleCloudPlatform/google-cloud-node/jobs/164763097 -- is it possible to correct these, or do we need to disable linting on these files?

// @jmuk

@landrito
Copy link
Contributor Author

landrito commented Oct 4, 2016

Looks to be a bug on our toolchain. I'll make some quick hand edits to this to match the bugfix.

@stephenplusplus
Copy link
Contributor

A quick win might be not naming the anonymous function set on the prototype:

-Service.prototype.method = function method() {
+Service.prototype.method = function() {

@landrito
Copy link
Contributor Author

landrito commented Oct 4, 2016

Actually, the linting errors (aside from the trailing space that I added) require a little more thought than I had previously anticipated. @jmuk thoughts on how this should be fixed?

@jmuk
Copy link
Contributor

jmuk commented Oct 4, 2016

@landrito: The lint happens on the variable names only, so

function (project, metricDescriptorPath) {
  return METRIC_DESCRIPTOR_PATH_PATH_TEMPLATE.render({
    project: project,
    metric_descriptor_path: metricDescriptorPath
  });
}

would be okay with jscs (it doesn't look great though). Changing the field name for metric_descriptor_path would be tough.

@landrito
Copy link
Contributor Author

landrito commented Oct 4, 2016

I see. I'll make the changes for the variable names and anonymous functions to toolkit and regenerate then. Thanks for looking into this.

@landrito
Copy link
Contributor Author

landrito commented Oct 5, 2016

@stephenplusplus. Regenerated! PTAL.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 9efe9b1 on landrito:monitoring into * on GoogleCloudPlatform:master*.

@stephenplusplus
Copy link
Contributor

Thanks!

@callmehiphop it looks like there's a error with parsing the docs, can you take a look? https://ci.appveyor.com/project/GoogleCloudPlatform/google-cloud-node/build/1.0.281/job/4k6cbwvjoh2mg4a4#L723

@landrito
Copy link
Contributor Author

Ping.

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

@googlebot googlebot added cla: no This human has *not* signed the Contributor License Agreement. and removed cla: yes This human has signed the Contributor License Agreement. labels Oct 12, 2016
@stephenplusplus
Copy link
Contributor

I added a test and more docs around how this is used.

Since npm install @google-cloud/monitoring doesn't work yet, a separate PR will be sent to document and expose the Monitoring API from the bundled package.

@callmehiphop I'm getting tripped up trying to get npm test to pass. PTAL and let me know if we should ignore or address that.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 33bdc64 on landrito:monitoring into * on GoogleCloudPlatform:master*.

@callmehiphop
Copy link
Contributor

Sorry, I missed this! Looks like the Monitoring title is missing from the overview config file.

@stephenplusplus
Copy link
Contributor

Added it there, but new error:

> npm run docs && npm run snippet-test && npm run unit-test


> google-cloud-node@ docs /Users/stephen/dev/gcloud-node
> node ./scripts/docs/packages.js

/Users/stephen/dev/gcloud-node/scripts/docs/packages.js:112
          throw err;
          ^

Error: ENOENT: no such file or directory, open 'docs/json/monitoring/master/index.json'
    at Error (native)

Feel free to check out this PR and get it working. You can push to landritos fork directly.

@callmehiphop
Copy link
Contributor

The service folders within docs/json are not generated. For whatever reason we keep them checked into git via .gitkeep files. It looks like you just need to make the monitoring/master folder.

@stephenplusplus
Copy link
Contributor

Woo, that did it! Ironically, this is the exact reason we have .gitkeep.

@callmehiphop
Copy link
Contributor

Awesome! As a side note, maybe we should look into generating the folders? I'm not sure I really see a need to keep them checked into git.

@stephenplusplus
Copy link
Contributor

Sure, generating them is fine.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling c053805 on landrito:monitoring into dd5a482 on GoogleCloudPlatform:master.

@stephenplusplus stephenplusplus removed the cla: no This human has *not* signed the Contributor License Agreement. label Oct 18, 2016
@stephenplusplus stephenplusplus merged commit f5f09ad into googleapis:master Oct 18, 2016
@stephenplusplus stephenplusplus added the api: monitoring Issues related to the Cloud Monitoring API. label Oct 21, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: monitoring Issues related to the Cloud Monitoring API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants