-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Fix iot test by using new method name #1528
Fix iot test by using new method name #1528
Conversation
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.
LGTM, thank you for the fix! I was trying to decide whether the right thing to do was to revise the client library or fix the sample.
Tests are passing for me locally, merging when green. |
I will see about getting the breaking change rolled back. |
I'll see if we can revert the breaking change |
does it make sense to resolve the conflicting file? @gguuss |
Yes, let's just go ahead and allow the breaking change in this case, I'll try and track down the other code samples that are out there requiring changes. |
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.
LGTM.
@mesmacosta Thank you so much for making the PR; we were able to get this resolved in the update to the IoT Client library version 1.3.1 so I'm going to go ahead and close this, favoring skipping the additional record of the breaking change as done in this PR. |
I noticed that this test started breaking in another PR that I opened.
Doing a quick analysis:
The iot/manager test failing seem to be related to this change:
googleapis/nodejs-iot@68a1d6e
Method name changed, but test from sample code was not updated.
client.registryPath -> client.deviceRegistryPath
So I can't say for sure, because I don't know the full extend of that commit, but this small change might fix it, I'd like to run it in the integrated environment to see if it will solve.
@fhinkel