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.

Sign up for GitHub

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

Jump to bottom

Refactor run-nodered script #816

Merged
merged 10 commits into from
Oct 30, 2024
Merged

Refactor run-nodered script #816

merged 10 commits into from
Oct 30, 2024

Conversation

andistorm
Copy link
Contributor

@andistorm andistorm commented Aug 6, 2024 •
edited
Loading

Describe your changes

  • Use docker volume to mount nodered config
  • Use nodered image from everest-dev-environment

Issue ticket number and link

Checklist before requesting a review

  • I have performed a self-review of my code
  • I have made corresponding changes to the documentation
  • I read the contribution documentation and made sure that my changes meet its requirements

Requires

@andistorm andistorm self-assigned this Aug 6, 2024
@andistorm andistorm marked this pull request as draft August 6, 2024 06:31
a-w50
a-w50 approved these changes Aug 7, 2024
View reviewed changes
cmake/assets/run_nodered_template.sh.in Outdated Show resolved Hide resolved
@andistorm
Refactor run-nodered script
c6e6508 
Signed-off-by: Andreas Heinrich <[email protected]>
@andistorm
Add comments to the nodered script
d3a67c0 
Signed-off-by: Andreas Heinrich <[email protected]>
@andistorm andistorm marked this pull request as ready for review October 7, 2024 10:03
@andistorm andistorm assigned hikinggrass and Pietfried and unassigned andistorm Oct 7, 2024
@andistorm andistorm mentioned this pull request Oct 8, 2024
Closed
a-w50
a-w50 reviewed Oct 10, 2024
View reviewed changes
cmake/assets/run_nodered_template.sh.in Outdated Show resolved Hide resolved
cmake/assets/run_nodered_template.sh.in Outdated
# allow starting the nodered container from inside a devcontainer

# Create docker volume to contain nodered config
docker volume create nodered-config-volume
Copy link
Contributor

Choose a reason for hiding this comment

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

nodered-config-volume is quite generic, not sure if it might collide with other node red instances. Would it be possible to use a local path inside build for example which gets mounted into the container instead of an volume. This would also ease the access to the config file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is more or less the way how the script worked before, I changed it to be able to start this container from inside a devcontainer, see below.

I changed the name of the volume and temporarily container to have a prefix everest-

cmake/assets/run_nodered_template.sh.in
Comment on lines +1 to +2
# In the following a volume is created to contain the nodered config, this is done to
# allow starting the nodered container from inside a devcontainer
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does using a volume allow to start the node-red container from inside a dev container?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The workspace inside the devcontainer is mounted to an other path. So you would need to know about the host path of the flow file.

docker run is executed on host with host paths

docker cp is executed with container's paths

This way it is possible to copy a file with only knowing the devcontainer paths into a docker volume which then can be mounted by name into the nodered container

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, well this is really ugly. These docker inside docker scenarios are quite special, not sure whether we should support these - they might break quite easily. This script here basically only works, if the devcontainer has been launched correctly - which is an assumption I wouldn't necessarily make.
Does this script work, when executed from the host (i.e. outside of the container?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is not real docker inside docker, because it is more like remote controlling the hosts docker socket.
This script is designed to work inside and outside the devcontainer. Of course it needs to be rebuild when changed because of paths

@andistorm andistorm mentioned this pull request Oct 11, 2024
Closed
2 tasks
@andistorm andistorm linked an issue Oct 21, 2024 that may be closed by this pull request
Node version error for Node-red UI table for nodered-sil.sh script #922
Closed
@hikinggrass
Copy link
Contributor

I'm getting the following error when running a nodered runscript (in this case ./build/run-scripts/nodered-sil-two-evse.sh) after a fresh build from this branch:

Status: Downloaded newer image for ghcr.io/everest/everest-dev-environment/nodered:docker-images-v0.1.0

> [email protected] start /usr/src/node-red
> node $NODE_OPTIONS node_modules/node-red/red.js $FLOWS "--userDir" "/data"

internal/fs/utils.js:332
    throw err;
    ^

Error: EACCES: permission denied, copyfile '/usr/src/node-red/node_modules/node-red/settings.js' -> '/data/settings.js'
    at Object.copyFileSync (fs.js:2062:3)
    at copyFile (/usr/src/node-red/node_modules/fs-extra/lib/copy-sync/copy-sync.js:70:6)
    at onFile (/usr/src/node-red/node_modules/fs-extra/lib/copy-sync/copy-sync.js:56:25)
    at getStats (/usr/src/node-red/node_modules/fs-extra/lib/copy-sync/copy-sync.js:48:44)
    at handleFilterAndCopy (/usr/src/node-red/node_modules/fs-extra/lib/copy-sync/copy-sync.js:33:10)
    at Object.copySync (/usr/src/node-red/node_modules/fs-extra/lib/copy-sync/copy-sync.js:26:10)
    at Object.<anonymous> (/usr/src/node-red/node_modules/node-red/red.js:129:20)
    at Module._compile (internal/modules/cjs/loader.js:1085:14)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1114:10)
    at Module.load (internal/modules/cjs/loader.js:950:32) {
  errno: -13,
  syscall: 'copyfile',
  code: 'EACCES',
  path: '/usr/src/node-red/node_modules/node-red/settings.js',
  dest: '/data/settings.js'
}
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! [email protected] start: `node $NODE_OPTIONS node_modules/node-red/red.js $FLOWS "--userDir" "/data"`
npm ERR! Exit status 1
npm ERR! 
npm ERR! Failed at the [email protected] start script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

@andistorm
Copy link
Contributor Author

I'm getting the following error when running a nodered runscript (in this case ./build/run-scripts/nodered-sil-two-evse.sh) after a fresh build from this branch:

Status: Downloaded newer image for ghcr.io/everest/everest-dev-environment/nodered:docker-images-v0.1.0

> [email protected] start /usr/src/node-red
> node $NODE_OPTIONS node_modules/node-red/red.js $FLOWS "--userDir" "/data"

internal/fs/utils.js:332
    throw err;
    ^

Error: EACCES: permission denied, copyfile '/usr/src/node-red/node_modules/node-red/settings.js' -> '/data/settings.js'
    at Object.copyFileSync (fs.js:2062:3)
    at copyFile (/usr/src/node-red/node_modules/fs-extra/lib/copy-sync/copy-sync.js:70:6)
    at onFile (/usr/src/node-red/node_modules/fs-extra/lib/copy-sync/copy-sync.js:56:25)
    at getStats (/usr/src/node-red/node_modules/fs-extra/lib/copy-sync/copy-sync.js:48:44)
    at handleFilterAndCopy (/usr/src/node-red/node_modules/fs-extra/lib/copy-sync/copy-sync.js:33:10)
    at Object.copySync (/usr/src/node-red/node_modules/fs-extra/lib/copy-sync/copy-sync.js:26:10)
    at Object.<anonymous> (/usr/src/node-red/node_modules/node-red/red.js:129:20)
    at Module._compile (internal/modules/cjs/loader.js:1085:14)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1114:10)
    at Module.load (internal/modules/cjs/loader.js:950:32) {
  errno: -13,
  syscall: 'copyfile',
  code: 'EACCES',
  path: '/usr/src/node-red/node_modules/node-red/settings.js',
  dest: '/data/settings.js'
}
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! [email protected] start: `node $NODE_OPTIONS node_modules/node-red/red.js $FLOWS "--userDir" "/data"`
npm ERR! Exit status 1
npm ERR! 
npm ERR! Failed at the [email protected] start script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

I added comamnd to set the UID and GID of the docker volume, could you try again pelase?

@andistorm andistorm closed this Oct 28, 2024
@andistorm andistorm reopened this Oct 28, 2024
@hikinggrass
Copy link
Contributor

I added comamnd to set the UID and GID of the docker volume, could you try again pelase?

This seems to have fixed it 👍

@Pietfried Pietfried merged commit 55fbf4d into main Oct 30, 2024
10 checks passed
@Pietfried Pietfried deleted the refactor/run-nodered-script branch October 30, 2024 10:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@a-w50 a-w50 a-w50 approved these changes

@hikinggrass hikinggrass hikinggrass approved these changes

@corneliusclaussen corneliusclaussen Awaiting requested review from corneliusclaussen corneliusclaussen is a code owner

@Pietfried Pietfried Awaiting requested review from Pietfried Pietfried is a code owner

Assignees

@hikinggrass hikinggrass

@Pietfried Pietfried

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Node version error for Node-red UI table for nodered-sil.sh script
4 participants
@andistorm @hikinggrass @a-w50 @Pietfried