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

Allow user setting node red version setting for application assigned devices #3766

Merged
merged 17 commits into from
May 3, 2024

Conversation

Steve-Mcl
Copy link
Contributor

@Steve-Mcl Steve-Mcl commented Apr 23, 2024

NOTE: Part 1 of 2. Only merge when both this and the device agent counterpart are both approved.

Description

As per Approach in issue discussion:

  1. Add editor side tab to device->settings
  2. Get/Set an object editor in DeviceSettings Table
    1. this provides a place for future editor based settings
  3. Override the starter & actual snapshots .modules['node-red'] version if editor.nodeRedVersion is set
    1. This acts like an override of any snapshot applied to the device
    2. Clearing the value reverts to using the version in the snapshot (latest for starter/whatever was stack was used for the snapshot)

further details and considerations are written in the linked issue

Related Issue(s)

Closes #2802

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 linked an issue Apr 23, 2024 that may be closed by this pull request
Copy link

codecov bot commented Apr 24, 2024

Codecov Report

Attention: Patch coverage is 57.14286% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 79.14%. Comparing base (b34cf65) to head (6090302).

Files Patch % Lines
forge/routes/api/deviceLive.js 57.14% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3766      +/-   ##
==========================================
- Coverage   79.15%   79.14%   -0.02%     
==========================================
  Files         278      278              
  Lines       12494    12501       +7     
  Branches     2771     2773       +2     
==========================================
+ Hits         9890     9894       +4     
- Misses       2604     2607       +3     
Flag Coverage Δ
backend 79.14% <57.14%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Steve-Mcl
Copy link
Contributor Author

Steve-Mcl commented Apr 24, 2024

Arrrrgggghhhh

unrelated e2e test still failing

  1) FlowForge - Team Overview (Home) - With License
       shows a list of cloud hosted instances:
     AssertionError: Timed out retrying after 4000ms: Expected to find content: 'application-device-a' within the element: <li> but never did.
      at Context.eval (webpack:///../test/e2e/frontend/cypress/tests-ee/team/overview.spec.js:123:27)

Tests run cleanly in local env with cypress open. will try full run without cypress open locally - perhaps something related to other tests is causing order of elements to be different?

UPDATE:

Fails with updated test since ordering of the devices differs depending on running full suite vs running single test.
i.e. Device named application-device-a is first in the list when the test is ran as part of full suite and application-device-b is first in the list when the test is ran standalone.

Locally, I have reverted the test modification to original which was failing like this:

  1) FlowForge - Team Overview (Home) - With License
       shows a list of cloud hosted instances:
     AssertionError: Timed out retrying after 4000ms: Expected to find element: `[data-action="open-editor"]`, but never found it.
      at Context.eval (webpack:///../test/e2e/frontend/cypress/tests-ee/team/overview.spec.js:126:62)

and ran the full suite and it works (arrrgggghhh No. 2!)

Will push original test momentarily.

Copy link
Member

@knolleary knolleary left a comment

Choose a reason for hiding this comment

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

As this has some dependency on the agent version, I think we should inform the user what behaviour they will get based on the known agent version.

  • older agents: NR version will only get applied when a new snapshot is deployed
  • new agents: NR version will get updated after settings saved (and/or new snapshot is deployed).

Exactly what version number to test for depends on us releasing the Device Agent - which we can do first

@Steve-Mcl
Copy link
Contributor Author

Exactly what version number to test for depends on us releasing the Device Agent - which we can do first

  • older agents: NR version will only get applied when a new snapshot is deployed
  • new agents: NR version will get updated after settings saved (and/or new snapshot is deployed).

@Steve-Mcl
For the personal memory banks - reminder to follow up once new release of device agent is cut

Add feedback when user applies a NR version to the device - so that they know when/how the set version will be applied (based on the agent version)

@Steve-Mcl
Copy link
Contributor Author

Steve-Mcl commented May 2, 2024

Since Agent is currently v2.4.0 and #254 has been merged, i can use a semvar <= 2.4.0 to indicate the alternative meeans of applying the NR version update.

Here is a demo - lay out was discussed with @joepavitt

chrome_WiS2CuKSuS

Footnotes:

agent version footnote
<= 2.4.0 The device will be updated to the specified version when a different snapshot is deployed
> 2.4.0 The device will be updated to the specified version and restarted upon saving the settings

Copy link
Member

@knolleary knolleary left a comment

Choose a reason for hiding this comment

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

Approved - with just one small rewording suggestion

Co-authored-by: Nick O'Leary <nick.oleary@gmail.com>
@knolleary
Copy link
Member

Only pause is now that this setting doesn't get applied if the device is in developer mode. Lets move forward with this for now, but I think we need a step back look at developer mode and how we communicate what does/doesn't happen whilst in that state and how we can improve things. For example, we don't let a pipeline stage get triggered from a developer-mode device.... which feels odd if its your development device you want to snapshot and deploy... but I digress and will follow up separately.

@knolleary knolleary merged commit 8cd145d into main May 3, 2024
10 of 11 checks passed
@knolleary knolleary deleted the 2802-device-node-red-version branch May 3, 2024 12:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Define Node-RED version for Devices not bound to an Instance
3 participants