-
Notifications
You must be signed in to change notification settings - Fork 69
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
Add iotedgehubdev integration #233
Conversation
Please also update the container to includes iotedgehubdev instead of the old ctl. Work with @yorek on coordinating that as he has a PR out to improve the way the containers are built. @yorek - I'm thinking that instead of running the full agent in the container, we just run iotedgehubdev (simulator) - because the container is only dev scenarios anyway. So, @LazarusX should be able to do a PR into your PR that just swaps the old ctl for iotedgehubdev. |
Sounds good to me, I may need some help with iotedgehubdev, never used before. |
@LazarusX - I get this when I run
Looks like I need to run setup first, which I did, then got this:
|
@LazarusX - Please also write some tests for this. Thanks! |
@LazarusX - Can runtime.py now be removed? |
@yorek You can see a quickstart instruction for iotedgehubdev at https://pypi.org/project/iotedgehubdev/ (more detailed documentations are coming soon). Please file issues at https://github.com/Azure/iotedgehubdev/issues. Let me know if you encounter any issues. |
Yes. You need to run setup before start to generate credentials. I am updating help messages to make it clear.
It seems that you didn't build the solution before running based on the image name |
@LazarusX - In start, can you automatically detect if setup hasn't been called and call it for the user? In start can you detect that it hasn't been build and let them know. The error wasn't enough to get me there. Thanks, Jon |
That's a bug with iotedgehubdev. I have filed issue Azure/iotedgehubdev#69.
I have added the check. |
@jongio I have add commits to resolve your comments. Please help review again. |
86ee312
to
3cee8dc
Compare
Get the following with tox
|
tests/test_simulator.py
Outdated
assert 'Starting event monitor' in out | ||
else: | ||
assert 'Monitoring events from device' in out | ||
assert 'timeCreated' in out |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bad merge. The last line should be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests/test_simulator.py
is added by me. The timeCreated
line is used to check whether we can successfully send D2C messages using iotedgehubdev.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Please increase the timeout to more than 2 seconds as well. Something like 15 seconds should be enough.
@jongio That's weird. What's the result of running |
@LazarusX - I ran this: iotedgedev solution create
I checked config\deployment.json and see the ${} syntax in the file. I rerun build and then the config\deployment.json file has the right settings. I traced it down. For some reason, the 'deploy' step is not expanding the envvars to the config file correctly. So run the steps above. Run the deploy step and then look at config\deployment.json |
@jongio This should be fixed now. Please try again. I also added more commits. |
Due to the issue I mentioned in #236 (comment), if tox stuck after starting the simulator in solution mode for Python 3.6, you might need to open another terminal and run |
For message: setup Setup IoT Edge simulator. This must be done before starting Does setup still need to be called before start? |
@LazarusX - I'm seeing this message: |
Yes. This is an issue with iotedgehubdev Azure/iotedgehubdev#69. Currently, iotedgehubdev does't provide a method to tell if the user has setup. |
@jongio Conflict resolved. |
Before I can merge this, we'll need to figure out how to kill monitor-process. Can you think of other options that would be a quick fix? For example, can you call stop once you have verified that messages are received? Or set a timer and call stop when it expires? Or only send a few messages? I also have a branch where @digimaun and I are trying to figure out how to kill the process, but tox is not running there. I'll work on that today. But a quick fix for you to unblock this merge would be ideal. Jon |
@jongio Do you mean temporarily getting this PR pass the tox e2e tests, or making the |
We need to be able to automate tox. So opening another window is not an option. Sure, get some sleep :) |
c5ba801
to
840b49c
Compare
This PR closes #202