Update supervisor methods to handle MC supervisor changes #509
Conversation
It's not supported in recent supervisors (>= 7.0.0) Change-Type: major
Change-Type: major
They're not supported on devices with more than one container. Change-Type: patch
Change-Type: minor
lib/models/device.coffee
Outdated
| @@ -895,6 +905,10 @@ getDeviceModel = (deps, opts) -> | |||
| # @function | |||
| # @memberof resin.models.device | |||
| # | |||
| # @deprecated | |||
| # This is not supported on multicontainer devices, and will | |||
| # be removed in future | |||
There was a problem hiding this comment.
NIT: removed in future --> removed in the future or removed in a future release
lib/models/device.coffee
Outdated
| @@ -849,6 +855,10 @@ getDeviceModel = (deps, opts) -> | |||
| # @function | |||
| # @memberof resin.models.device | |||
| # | |||
| # @deprecated | |||
| # This is not supported on multicontainer devices, and will | |||
| # be removed in future | |||
There was a problem hiding this comment.
NIT: removed in future --> removed in the future or removed in a future release
| device.supervisor_version, | ||
| MIN_SUPERVISOR_MC_API | ||
| ).then -> | ||
| appId = device.belongs_to__application[0].id |
There was a problem hiding this comment.
I've started thinking about moving the getExpanded,... util methods to the SDK.
There was a problem hiding this comment.
I'm not going to do that right now, but in general I do think you're right. Really imo, those should live in Pine, as nice helper methods for anybody using the pine client.
There was a problem hiding this comment.
Yea sure, never meant to have this in this PR :)
| # @memberof resin.models.device | ||
| # | ||
| # @param {String|Number} uuidOrId - device uuid (string) or id (number) | ||
| # @param {Number} imageId - id of the image to start |
There was a problem hiding this comment.
After reading Start service on device, I would expect this to be the service ID 😕
Not-for-now: Wouldn't it be great if we could handle the service name as well?
I guess that getting the currently running image from the service name & deviceId isn't straightforward, but I imagine that the consumers might need to do this on their own.
There was a problem hiding this comment.
It's not that easy. The main problem is that it's possible to have two images from the same service running on one device at the same time, if you're using handovers, or plausibly if something goes wrong somewhere or something. That's important because it's a risky case, and it's a time when you're mostly likely to want to manually kill one of your containers (because your handover has got stuck).
This model is also a slightly nicer in that it makes it hard to hit race conditions, where you try to stop one image while another is downloading and end up stopping a different one, because the supervisor is changing things at the same time you're talking to it. You can avoid that if you specify image ids, but if you can only use services then there's nothing you can do to be more specific.
All that said, I can see it's still not super convenient. What do you think about startService + startServiceByName? The 2nd would do what you're suggesting (and throw an error if it's ambiguous), but we'd also have the 1st option so you can disambiguate when it matters.
There was a problem hiding this comment.
All that said, I can see it's still not super convenient. What do you think about startService + startServiceByName? The 2nd would do what you're suggesting (and throw an error if it's ambiguous), but we'd also have the 1st option so you can disambiguate when it matters.
Sounds great 👍 . I also don't think that this need to block this PR. We could do it in a separate minor release.
There was a problem hiding this comment.
Created an issue for later: #512
lib/models/device.coffee
Outdated
| @@ -971,6 +985,159 @@ getDeviceModel = (deps, opts) -> | |||
| .catch(notFoundResponse, treatAsMissingDevice(uuidOrId)) | |||
| .asCallback(callback) | |||
|
|
|||
| ###* | |||
| # @summary Start service on device | |||
There was a problem hiding this comment.
NIT: How about Start a service on a device?
lib/models/device.coffee
Outdated
| .asCallback(callback) | ||
|
|
||
| ###* | ||
| # @summary Stop service on device |
There was a problem hiding this comment.
NIT: How about Stop a service on a device?
lib/models/device.coffee
Outdated
| .asCallback(callback) | ||
|
|
||
| ###* | ||
| # @summary Restart service on device |
There was a problem hiding this comment.
NIT: How about Restart a service on a device?
|
One question left to think about image ids vs service names, but I've pushed changes to fix all the other comments. |
|
@pimterry, status checks have failed for this PR. Please make appropriate changes and recommit. |
Also removed an existing deprecated method - if we're breaking things, might as well clean up a bit.