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

Updates client library, cleans up JS, fixes package.js so the tests run #698

Merged
merged 4 commits into from
Jul 31, 2018

Conversation

gguuss
Copy link
Contributor

@gguuss gguuss commented Jul 30, 2018

No description provided.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jul 30, 2018
@codecov
Copy link

codecov bot commented Jul 30, 2018

Codecov Report

Merging #698 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #698   +/-   ##
=======================================
  Coverage   55.17%   55.17%           
=======================================
  Files           1        1           
  Lines          58       58           
=======================================
  Hits           32       32           
  Misses         26       26

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9c15971...ec208be. Read the comment docs.

@gguuss gguuss requested a review from fhinkel July 30, 2018 18:24
@gguuss
Copy link
Contributor Author

gguuss commented Jul 30, 2018

Adds the following test fixes:

  • Updates the client library to the latest (the old version I used is incompatible with GCE)
  • Updates the MQTT sample to have test scripts so it actually runs tests

@@ -104,7 +104,7 @@ function lookupRegistry (client, registryId, projectId, cloudRegion, cb) {
console.log(err);
} else {
console.log('Looked up existing registry');
console.log(data);
console.log(data.data);
Copy link
Contributor

Choose a reason for hiding this comment

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

Calling this data.data is confusing. The callback argument should be named res, or response, as it contains an HTTP response object.

const discoveryUrl =
`${DISCOVERY_API}?version=${API_VERSION}`;

if (authClient.createScopedRequired && authClient.createScopedRequired()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need to do this :) Just pass the list of scopes into the getClient method:

google.auth.getClient({
  scopes: ['https://www.googleapis.com/auth/cloud-platform']
}).then(auth => {
  google.options({ auth });
  google.discoverAPI(....
});

"ava": "0.25.0",
"jsonwebtoken": "8.2.0",
"mqtt": "2.16.0",
"uuid": "3.2.1",
"yargs": "11.0.0"
},
"testDependencies": {},
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't believe testDependencies are a thing :)

auth: authClient
});

google.discoverAPI(discoveryUrl).then((client, err) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think the second param here would be an error. That's traditionally where you would use a .catch() :)

"dependencies": {
"@google-cloud/pubsub": "0.16.4",
"@google-cloud/nodejs-repo-tools": "2.2.1",
"@google-cloud/nodejs-repo-tools": "1.4.17",
Copy link
Contributor

Choose a reason for hiding this comment

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

why downgrade repo tools?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

samples command is in 1.x? This affects the packages.json syntax for test -- couldn't find one for 2.2.1

Copy link
Contributor

Choose a reason for hiding this comment

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

@jmdobry whatup?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I blame myself, I didn't really look very hard.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Found it, will commit with update to runner / repo tools

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@gguuss gguuss merged commit 146fdd0 into master Jul 31, 2018
@gguuss gguuss deleted the updatelib branch July 31, 2018 23:58
ace-n pushed a commit that referenced this pull request Nov 17, 2022
Change-Id: I4e604288729e77a2bc8a31c749a89ee662cb3ff8

Fixes #698  🦕
ace-n pushed a commit that referenced this pull request Nov 17, 2022
Change-Id: I4e604288729e77a2bc8a31c749a89ee662cb3ff8

Fixes #698  🦕
ace-n pushed a commit that referenced this pull request Nov 17, 2022
Change-Id: I4e604288729e77a2bc8a31c749a89ee662cb3ff8

Fixes #698  🦕
ace-n pushed a commit that referenced this pull request Nov 17, 2022
Change-Id: I4e604288729e77a2bc8a31c749a89ee662cb3ff8

Fixes #698  🦕
ace-n pushed a commit that referenced this pull request Nov 17, 2022
Change-Id: I4e604288729e77a2bc8a31c749a89ee662cb3ff8

Fixes #698  🦕
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.

3 participants