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

Release Resin-SDK v7.0.0 #420

Merged
23 commits merged into from Oct 12, 2017
Merged

Release Resin-SDK v7.0.0 #420

23 commits merged into from Oct 12, 2017

Conversation

pimterry
Copy link
Contributor

@pimterry pimterry commented Oct 12, 2017

This includes lots of breaking changes, notably including the upgrade to the new API v3.

All these changes have previously been reviewed, this PR is just merging them all together and using versionbot to actually ship them to npm.

For posterity, this PR includes:

…length

These were slightly convenient, but hacky and inconsistent with any data
retrieved manually with $expands. One day we might bring these back, but
for now it's much cleaner for everybody if we keep this simple.

Change-Type: major
Both of these were generated from expanded data we'd like to remove, and
somewhat messily at that. You can easily generate them by hand by
expanding application yourself and looking at the name and
id there by hand.

Change-Type: major
….' } options to opt in instead.

Change-Type: major
Breaking: Don't allow creating devices with discontinued device types
Node 4 may well still work with the SDK for quite a while, but we'll no
longer actively test against it, and it's quite possible that it may
stop working entirely in any future release.

Change-Type: major
Replace generateApiKey with generateProvisioningKey
*BREAKING*: Stop actively supporting node 4.
…& result properties now include verbs (e.g. device.application is now device.belongs_to__application).

Change-Type: major
@thgreasi
Copy link
Member

Woohoo! I'm on it!

@pimterry
Copy link
Contributor Author

@thgreasi I've been doing my own review of the codebase - I think I've actually found a couple of bugs in here in the untested device methods (things that do stuff with real devices, like shutdown/reboot) from the combination of changing the relationships and removing the expands. I'm going to do some quick testing to confirm, fix them, and then update this PR with the results soon.

@ghost
Copy link

ghost commented Oct 12, 2017

@pimterry, status checks have failed for this PR. Please make appropriate changes and recommit.

@@ -421,7 +428,7 @@ getDeviceModel = (deps, opts) ->
exports.getApplicationInfo = (uuidOrId, callback) ->
exports.get(uuidOrId).then (device) ->
ensureSupervisorCompatibility(device.supervisor_version, MIN_SUPERVISOR_APPS_API).then ->
appId = device.application[0].id
appId = device.belongs_to__application[0].id
Copy link
Member

Choose a reason for hiding this comment

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

I think that this needs to change since we are not expanding the belongs_to__application.
I guess that we should either expand it or use device.belongs_to__application.__id.

@@ -812,7 +819,7 @@ getDeviceModel = (deps, opts) ->
exports.startApplication = (uuidOrId, callback) ->
exports.get(uuidOrId).then (device) ->
ensureSupervisorCompatibility(device.supervisor_version, MIN_SUPERVISOR_APPS_API).then ->
appId = device.application[0].id
appId = device.belongs_to__application[0].id
Copy link
Member

Choose a reason for hiding this comment

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

Like above

I think that this needs to change since we are not expanding the belongs_to__application.
I guess that we should either expand it or use device.belongs_to__application.__id.

@@ -855,7 +862,7 @@ getDeviceModel = (deps, opts) ->
exports.stopApplication = (uuidOrId, callback) ->
exports.get(uuidOrId).then (device) ->
ensureSupervisorCompatibility(device.supervisor_version, MIN_SUPERVISOR_APPS_API).then ->
appId = device.application[0].id
appId = device.belongs_to__application[0].id
Copy link
Member

Choose a reason for hiding this comment

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

Like above

I think that this needs to change since we are not expanding the belongs_to__application.
I guess that we should either expand it or use device.belongs_to__application.__id.

@@ -982,7 +989,7 @@ getDeviceModel = (deps, opts) ->
baseUrl: apiUrl
body:
deviceId: device.id
appId: device.application[0].id
appId: device.belongs_to__application[0].id
Copy link
Member

Choose a reason for hiding this comment

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

Like above

data:
appId: device.application[0].id
appId: device.belongs_to__application[0].id
Copy link
Member

Choose a reason for hiding this comment

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

Like above twice

@@ -1071,7 +1078,7 @@ getDeviceModel = (deps, opts) ->
baseUrl: apiUrl
body:
deviceId: device.id
appId: device.application[0].id
appId: device.belongs_to__application[0].id
Copy link
Member

@thgreasi thgreasi Oct 12, 2017

Choose a reason for hiding this comment

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

Like above and actually I think that's the same for the rest of the occurrences in this file.
Sorry for the noise.

@pimterry
Copy link
Contributor Author

@thgreasi See the last 4 commits - I've been through and manually tested every untested method in the devices file with a real pi. I've fixed the few that were broken by missing relationships, plus fixing a couple of preexisting bugs and mess in some of the others.

I've also stopped exposing the weird ensureSupervisorCompatibility method (though we still use it internally), which just did a semver comparison and and then threw an exception, and didn't really know anything about supervisor versions at all.

Rereview please!

Copy link
Member

@thgreasi thgreasi left a comment

Choose a reason for hiding this comment

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

Have some questions but already LGTM 👍 🎊

return request.send
method: 'DELETE'
method: 'POST'
Copy link
Member

Choose a reason for hiding this comment

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

I guess that this changed at some point

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as I can tell, it's needed this since basically forever, so it's never worked... The API supervisor proxy requires a POST, plus the real method to use in the body.

Horia fixed the same issue in ping recently. The problem is we just don't have automated tests for this stuff right now, so any useful but rarely used methods like this that require real devices can be silently broken 😢

@@ -1070,8 +1085,13 @@ getDeviceModel = (deps, opts) ->
# if (error) throw error;
# });
###
exports.update = (uuidOrId, options, callback) ->
exports.get(uuidOrId).then (device) ->
exports.update = (uuidOrId, options = {}, callback) ->
Copy link
Member

Choose a reason for hiding this comment

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

What happens when updating with an empty object? Does it serve like a ping?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Update options are actually only allowed to be force: true/false (or nothing). All this method does is ping a device to get it to check for an update. I'm not 100% sure why that's useful, but it's a feature the supervisor supports, so I'm happy to leave it here for now.

@@ -101,7 +101,7 @@ getDeviceModel = (deps, opts) ->
# console.log('Is compatible');
# });
###
exports.ensureSupervisorCompatibility = ensureSupervisorCompatibility = Promise.method (version, minVersion) ->
ensureSupervisorCompatibility = Promise.method (version, minVersion) ->
Copy link
Member

Choose a reason for hiding this comment

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

It seems that we don't use it in the dashboard, so 👍

@ghost
Copy link

ghost commented Oct 12, 2017

VersionBot failed to carry out a status check for the above pull request here: #420. The reason for this is:
2 of 5 required status checks are pending.
Please carry out relevant changes or alert an appropriate admin.

@pimterry
Copy link
Contributor Author

pimterry commented Oct 12, 2017

Here we go!

@ghost ghost merged commit 25db78a into master Oct 12, 2017
@ghost ghost deleted the v7 branch October 12, 2017 16:03
This pull request was closed.
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.

None yet

2 participants