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

Send nr version #327

Merged
merged 11 commits into from
Dec 13, 2024
Merged

Send nr version #327

merged 11 commits into from
Dec 13, 2024

Conversation

hardillb
Copy link
Contributor

Description

Has the the device agent report the currently running Node-RED version as part of it's regular check-in

This for use with the Device Editor Proxy.

Related Issue(s)

FlowFuse/flowfuse#3414

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

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

@hardillb hardillb marked this pull request as ready for review December 13, 2024 09:53
@hardillb hardillb requested a review from Steve-Mcl December 13, 2024 10:08
Copy link
Contributor

@Steve-Mcl Steve-Mcl left a comment

Choose a reason for hiding this comment

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

Ben, I can see a few other places in the code base where getAgent is called but without the async. And where they are, they have sync function signatures meaning the proliferation of the async await stuff has tentacles that reach even further out & I am unsure of the sideeffects in production.

image

TBH, I would be tempted to undo all of this and simply modify launcher.readPackage() to use readFileSync from fs.

There are only around ~5 calls to readPackage that would simply need to lose the await - then there would be no need to make getState async and no need to follow its code branches and parent callers adding await to everything it touches.

@hardillb hardillb requested a review from Steve-Mcl December 13, 2024 11:05
lib/mqtt.js Outdated Show resolved Hide resolved
Co-authored-by: Stephen McLaughlin <[email protected]>
Copy link
Contributor

@Steve-Mcl Steve-Mcl left a comment

Choose a reason for hiding this comment

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

Yeah, much cleaner without all the proliferating async await.

However, there is an issue with module vs modules and a couple of suggestions.

Lastly, do we thing a unit test is possible?

there is already tests in test/unit/lib/agent_spec.js around line 493 for getState - even just the presence of the property being a valid semver

lib/agent.js Outdated Show resolved Hide resolved
lib/agent.js Outdated Show resolved Hide resolved
lib/agent.js Outdated Show resolved Hide resolved
Co-authored-by: Stephen McLaughlin <[email protected]>
@hardillb
Copy link
Contributor Author

hardillb commented Dec 13, 2024

Lastly, do we thing a unit test is possible?

there is already tests in test/unit/lib/agent_spec.js around line 493 for getState - even just the presence of the property being a valid semver

I don't see how without having the test actually install Node-RED.

I already had to patch the code because the tests are injecting a JSON object rather than a real launcher object

@Steve-Mcl
Copy link
Contributor

I don't see how without having the test actually install Node-RED.

I already had to patch the code because the tests are injecting a JSON object rather than a real launcher object

Fair enough Ben - just thought to ask.

@Steve-Mcl
Copy link
Contributor

can we look at using semver.valid since semver is already a dependency?

Without this, I can foresee a future ticket where the nodeRedVersion is * or dev :)

lib/agent.js Outdated Show resolved Hide resolved
@hardillb hardillb requested a review from Steve-Mcl December 13, 2024 14:28
@Steve-Mcl Steve-Mcl merged commit eacc069 into main Dec 13, 2024
4 checks passed
@Steve-Mcl Steve-Mcl deleted the send-nr-version branch December 13, 2024 14:29
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.

2 participants