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

Final additions in private beta #489

Merged
merged 11 commits into from
Sep 26, 2017
Merged

Final additions in private beta #489

merged 11 commits into from
Sep 26, 2017

Conversation

gguuss
Copy link
Contributor

@gguuss gguuss commented Sep 22, 2017

No description provided.

@gguuss gguuss added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Sep 22, 2017
Copy link
Member

@dizcology dizcology left a comment

Choose a reason for hiding this comment

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

Not sure how easy it would be to add tests for the codes in http_example/. If it is easy and makes sense to have them, please consider adding some tests.


# Google Cloud IoT Core NodeJS HTTP example

This sample app publishes messages to Cloud Pub/Sub or states using the HTTP
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand what "Cloud Pub/Sub or states" means.

@@ -0,0 +1,279 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

What is this file for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed, the device API was to be exposed as Apiary via device REST discovery.

description: 'Encryption algorithm to generate the JWT.',
requiresArg: true,
demandOption: true,
choices: ['RS256', 'ES256'],
Copy link
Member

Choose a reason for hiding this comment

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

If the target user of this sample is expected to be familiar with JWT, or node already prints allowed choices, then please ignore my comment:

It might help to print the allowed choices in the help/description message.

// messageCount. Telemetry events are published at a rate of 1 per second and
// states at a rate of 1 every 2 seconds.
function publishAsync (messageCount, numMessages) {
const payload = `${argv.registry_id}/${argv.device_id}-payload-${messageCount}`;
Copy link
Member

Choose a reason for hiding this comment

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

The publishAsync function in http_client_example.js seems more flexible allowing the user to specify a message argument. Why is the publishAsync function in this file not allowing the same?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Checked in duplicate versions of the same sample, will clean up after dinner.

console.log('Publishing to:', path);
console.log('Publishing message:', message);

// TODO: Publish message hurr
Copy link
Member

Choose a reason for hiding this comment

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

Still need to do this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed extra implementation,


// Publish numMessages messages asynchronously, starting from message
// messageCount.
function publishAsync (bearer, client, messageCount, message, projectId, cloudRegion, registryId, deviceId) {
Copy link
Member

Choose a reason for hiding this comment

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

The order of arguments is a bit inconsistent:

for this function: (bearer, client, messageCount, message, projectId, cloudRegion, registryId, deviceId)

for getDeviceState in manager.js it is (client, deviceId, registryId, projectId, cloudRegion)

Copy link
Contributor Author

@gguuss gguuss left a comment

Choose a reason for hiding this comment

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

This will be difficult to setup tests for because it depends on a lot of components being in place both in order to run the sample as well as being able to measure the sample is working.

@gguuss gguuss removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Sep 26, 2017
@gguuss gguuss merged commit 5d90464 into master Sep 26, 2017
@gguuss gguuss deleted the iot-snippets branch September 26, 2017 23:34
ace-n pushed a commit that referenced this pull request Nov 16, 2022
Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
ace-n pushed a commit that referenced this pull request Nov 17, 2022
Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
ahrarmonsur pushed a commit that referenced this pull request Nov 17, 2022
Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants