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

Include project nodes for app assigned devices #205

Merged
merged 3 commits into from
Nov 17, 2023

Conversation

Steve-Mcl
Copy link
Contributor

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)

FlowFuse/flowfuse#3018

Checklist

  • I have read the contribution guidelines
  • Suitable unit/system level tests have been added and they pass
  • Documentation has been updated
    • Upgrade instructions
    • Configuration details
    • Concepts
  • Changes flowforge.yml?
    • Issue/PR raised on FlowFuse/helm to update ConfigMap Template
    • Issue/PR raised on FlowFuse/CloudProject to update values for Staging/Production

Labels

  • Backport needed? -> add the backport label
  • Includes a DB migration? -> add the area:migration label

@Steve-Mcl Steve-Mcl changed the title 3018 project nodes for app assigned device Include project nodes for app assigned devices Nov 14, 2023
@Steve-Mcl Steve-Mcl requested a review from hardillb November 15, 2023 11:41
@hardillb hardillb merged commit 4261c82 into main Nov 17, 2023
2 checks passed
@hardillb hardillb deleted the 3018-project-nodes-for-app-assigned-device branch November 17, 2023 14:39
@knolleary
Copy link
Member

knolleary commented Nov 17, 2023

@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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

Steve-Mcl commented Nov 20, 2023

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.

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.

Project Nodes unavailable on a Device assigned to an Application
3 participants