search

FlowFuse / device-agent

An agent to run FlowFuse managed instances of Node-RED on devices
Apache License 2.0
15 stars 8 forks source link

Include project nodes for app assigned devices #205

Closed Steve-Mcl closed 9 months ago

Steve-Mcl commented 9 months ago

Description

This pull request adds project nodes for app assigned devices. The @flowfuse/nr-project-nodes package has been added to the package.json file, and the Launcher class has been updated to include the path to project nodes. NOTE, the settings.nodesDir (used to provide access to the project nodes for Node-RED) is conditionally added based on the env being EE and will include a different path to the @flowfuse/nr-project-nodes package in the dev-env when detected.

This pull request addresses the following commits:

Related Issue(s)

https://github.com/FlowFuse/flowfuse/issues/3018

Checklist

Labels

knolleary commented 9 months ago

@Steve-Mcl I'm not clear why some of the functionality in this PR is required. We already have the ability to use the project nodes in an instance-assigned device. What is different about an app-assigned device that requires all of these changes?

Likewise, we haven't needed nearly as much dev-env specific logic previously - what changed?

Steve-Mcl commented 9 months ago

Hi @knolleary

We already have the ability to use the project nodes in an instance-assigned device. What is different about an app-assigned device that requires all of these changes?

Ben and I had a chat about the best way to handle this. Since we do not yet have the ability to add packages (app device has no settings), we needed a way to get the project-nodes loaded.

The dev-env logic checks to see if there is a packages/nr-project-nodes or the NODE_ENV is development & instructs the agent to use the src instead.

Likewise, we haven't needed nearly as much dev-env specific logic previously - what changed?

There is another place in the code base where the same* dev-env logic is included. The logic in here is pretty much doing the exact same thing (adding to nodes dir) (albeit the code here looks like it is "more" as it was factored out to make it easy to unit test).

I am not at my computer ATM but I think the same* dev-env logic is in the regular launcher?

*same: I additionally test that NODE_ENV is "development"

knolleary commented 9 months ago

Thanks for reply @Steve-Mcl

Since we do not yet have the ability to add packages (app device has no settings), we needed a way to get the project-nodes loaded.

An alternative approach would have been to list them in the default snapshot we provide a device an app-mode.

https://github.com/FlowFuse/flowfuse/blob/13a7e55072df1359d949109cea5ee3c5e7651709/forge/routes/api/deviceLive.js#L105

I do want to make sure we don't have a problem with this PR merged but none of the others merged yet? It will be important these nodes don't get enabled when running against an older FF platform version because those platforms won't have the ACL support needed.

Steve-Mcl commented 9 months ago

An alternative approach would have been to list them in the default snapshot we provide a device an app-mode.

https://github.com/FlowFuse/flowfuse/blob/13a7e55072df1359d949109cea5ee3c5e7651709/forge/routes/api/deviceLive.js#L105

Ah, right, so the device agent would only de instructed to install project nodes when the user has updated the core (and the platform is licenced too) 🤔

Very good point. Pretty sure we can also evaluate the device agent version too to ensure it is compatible before including the package.

I will do some manual testing and determine if we revert this PR before we ship a device agent update.

Thanks.