Add the Python device and server files from the end-to-end-example #1218
Conversation
googlebot
added
the
cla: yes
|
@gguuss can you review this? |
|
I'll take a look when I can. This looks like a non-trivial amount of code without any tests so it will take me a little time to go through it. |
There was a problem hiding this comment.
- Missing
requirements.txt - Lots of lint errors. Please run
flake8on your code - Missing tests (if you can't add them, I can add some when I do this for the MQTT / HTTP clients)
- Please remove README.md use the script from scripts/readme-gen/readme_gen.py to generate from a template similar to the manager example
|
|
||
| def discovery_url(api_key): | ||
| """Construct the discovery url for the given api key.""" | ||
| return '{}?version={}&key={}'.format(DISCOVERY_API, API_VERSION, api_key) |
There was a problem hiding this comment.
API key is no longer necessary because the API is public
| description='Example of Google Cloud IoT registry and device management.') | ||
| # Required arguments | ||
| parser.add_argument( | ||
| '--project_id', required=True, help='GCP cloud project name.') |
There was a problem hiding this comment.
Any reason we're not using the GOOGLE_CLOUD_PROJECT or GCLOUD_PROJECT environment variables?
There was a problem hiding this comment.
Added GOOGLE_CLOUD_PROJECT default.
| '--pubsub_subscription', | ||
| required=True, | ||
| help='Google Cloud Pub/Sub subscription name.') | ||
| parser.add_argument('--api_key', required=True, help='Your API key.') |
There was a problem hiding this comment.
Same here, we usually default to the API_KEY or GOOGLE_API_KEY environment variables.
There was a problem hiding this comment.
Remove api_key argument.
| parser.add_argument('--api_key', required=True, help='Your API key.') | ||
|
|
||
| # Optional arguments | ||
| parser.add_argument( |
There was a problem hiding this comment.
We generally default to the GOOGLE_APPLICATION_CREDENTIALS environment variable for this
| data): | ||
| """Push the data to the given device as configuration.""" | ||
| config_data = None | ||
| print 'The device ({}) has a temperature of: {}'.format(device_id, |
There was a problem hiding this comment.
We generally avoid hanging indents like this
There was a problem hiding this comment.
Can you be more specific? Unless it's been fixed.
|
|
||
| # This is the topic that the device will receive configuration updates on. | ||
| mqtt_config_topic = '/devices/{}/config'.format(args.device_id) | ||
|
|
There was a problem hiding this comment.
It may make sense to acknowledge the messages using the devices/{}/state MQTT topic
| """Parse command line arguments.""" | ||
| parser = argparse.ArgumentParser( | ||
| description='Example Google Cloud IoT MQTT device connection code.') | ||
| parser.add_argument( |
There was a problem hiding this comment.
Maybe default some of these as described for the server:
- GCLOUD_PROJECT or GOOGLE_CLOUD_PROJECT - used for the project ID
- GOOGLE_API_KEY - used for API key (although the sample should not require it)
- GOOGLE_APPLICATION_CREDENTIALS - path to service.json
| $ python cloudiot_pubsub_example_server.py \ | ||
| --project_id=my-project-id \ | ||
| --pubsub_subscription=my-topic-subscription \ | ||
| --api_key=YOUR_API_KEY |
There was a problem hiding this comment.
Is API key still necessary?
…docs-samples into python-end-to-end-example Latest updates to device and server, and adding requirements.txt.
…docs-samples into python-end-to-end-example Fixing linter issues.
|
@jonparrott LGTM, can squash and merge when you're ready. |
pytest.ini
Outdated
| @@ -1,6 +0,0 @@ | |||
| [pytest] | |||
There was a problem hiding this comment.
Why are you deleting our pytest.ini?
There was a problem hiding this comment.
I thought it was a local file of mine for some reason.
| google-api-python-client==1.6.4 | ||
| google-auth-httplib2==0.0.3 | ||
| google-auth==1.2.1 | ||
| google-cloud==0.30.0 |
There was a problem hiding this comment.
Please don't pull in the broad google-cloud package, please include individual google-cloud-* packages that you actually need here.
|
@noerog @gguuss The system tests for this are failing: https://travis-ci.org/GoogleCloudPlatform/python-docs-samples/builds/313205503?utm_source=email&utm_medium=notification Can one of you send a follow-up to fix? |
|
I'll take a look. |
They should live in GitHub.