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

Allow a device to be assigned to application #161

Closed Steve-Mcl closed 1 year ago

Steve-Mcl commented 1 year ago

Description

Permit a device to be assigned to an application.

NOTE: Raised in draft to permit other engineers to test - before writing a whole bunch of tests and updating docs.

Notable:

  1. permit the agent to start even if the working dir does not exist. Instead of a hard exit, an attempt to generate the working directory is made.
  2. adds currentApplication which when set, permits the device to launch node-red - allowing user to modifiy flows.
  3. updates the log output to indicate instance or application as device owner
  4. only writes necessary parts to file - to prevent the fake application snapshot overwriting users flows edits

Application level device implementation:

TODO (this iteration):

TODO (next iteration)

Related Issue(s)

Devices to be assigned to an Application, without being assigned to an Instance#2619

Checklist

Labels

knolleary commented 1 year ago

In quite a few places with this PR there is pieces of logic similar to:

(this.currentApplication ? 'application' : (this.currentProject ? 'project' : 'none'))

In relation to the discussion on https://github.com/flowforge/flowforge/pull/2621#discussion_r1304446272 , I can see a possible future where the valid states will be.

Assigned To currentApplication currentProject
nothing null null
Application <applicationId> null
Instance <applicationId> <instanceId>

Meaning, the presence of application means its assigned to something, and instance tells you if it is a specific instance or not.

Looking at that line of code above, it puts all the weight on whether application is set or not. That matches up with the current implementation of the platform, but would make it harder if we decide to modify the model as I've described.

I think we could safely flip the logic in the device agent to something like:

this.currentProject ? ' project' : (this.currentApplication ? 'application' : 'none'))

I believe that would still behave the same way with the current server implementation, but would also work if we modified things.

Given how many places that bit of logic exists in this PR, I don't know if the logic is sound - but wanted to get your input if this is feasible?

Steve-Mcl commented 1 year ago

Looking at that line of code above, it puts all the weight on whether application is set or not. That matches up with the current implementation of the platform, but would make it harder if we decide to modify the model as I've described.

I think we could safely flip the logic in the device agent to something like:

this.currentProject ? ' project' : (this.currentApplication ? 'application' : 'none'))

I believe that would still behave the same way with the current server implementation, but would also work if we modified things.

Given how many places that bit of logic exists in this PR, I don't know if the logic is sound - but wanted to get your input if this is feasible?

The order of the logic was deliberate and considered but I also understand your point.

I will go through both ends again and determine the side-effects of changing (if any).

Watch this space.

Steve-Mcl commented 1 year ago

I think we could safely flip the logic in the device agent to something like:

this.currentProject ? ' project' : (this.currentApplication ? 'application' : 'none'))

I believe that would still behave the same way with the current server implementation, but would also work if we modified things. Given how many places that bit of logic exists in this PR, I don't know if the logic is sound - but wanted to get your input if this is feasible?

The order of the logic was deliberate and considered but I also understand your point.

I will go through both ends again and determine the side-effects of changing (if any).

Login order changed in 1026df4 and verified locally.

It did throw up an issue which has also been resolved on the platform repo.

Ready for re-review now.