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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
c6e6508
Refactor run-nodered script
andistorm Aug 6, 2024
d3a67c0
Add comments to the nodered script
andistorm Oct 7, 2024
439cfa1
Use debian:12-slim as temporarily container image
andistorm Oct 8, 2024
d18da86
Merge branch 'main' into refactor/run-nodered-script
andistorm Oct 8, 2024
796e0e1
update tag
andistorm Oct 21, 2024
ed71995
Add prefix
andistorm Oct 21, 2024
d7ddd8d
Merge branch 'main' into refactor/run-nodered-script
andistorm Oct 21, 2024
14ca168
set group and owner of docker volume
andistorm Oct 28, 2024
3de153e
Merge branch 'main' into refactor/run-nodered-script
andistorm Oct 28, 2024
e82d5ad
Merge branch 'main' into refactor/run-nodered-script
Pietfried Oct 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 0 additions & 6 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,12 +129,6 @@ endif()
# FIXME (aw): this should be optional
add_subdirectory(config)

# install docker related files
install(
DIRECTORY "cmake/assets/docker"
DESTINATION "${CMAKE_INSTALL_DATADIR}/everest"
)

ev_install_project()

# configure clang-tidy if requested
Expand Down
5 changes: 0 additions & 5 deletions cmake/assets/docker/Dockerfile
View file
Open in desktop

This file was deleted.

20 changes: 17 additions & 3 deletions cmake/assets/run_nodered_template.sh.in
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,17 @@
cd @EVEREST_DOCKER_DIR@
docker build -t everest-nodered .
docker run --rm --network host --name everest_nodered --mount type=bind,source=@FLOW_FILE@,target=/data/flows.json everest-nodered
# 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

Comment on lines +1 to +2
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

# 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-

# Create temporarily container to copy nodered config into the created volume
docker create -v nodered-config-volume:/data --name nodered-config-container debian:12-slim true

# Copy nodered config to the created volume with the temporarily created container
docker cp @FLOW_FILE@ nodered-config-container:/data/flows.json

a-w50 marked this conversation as resolved.
Show resolved Hide resolved
# Remove temporarily container
docker rm nodered-config-container

# Start nodered container with the volume mounted to /data
docker run --rm --network host --name everest_nodered --mount type=volume,source=nodered-config-volume,target=/data ghcr.io/everest/everest-dev-environment/nodered:v0.7.0
3 changes: 0 additions & 3 deletions cmake/config-run-nodered-script.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,6 @@ function(generate_nodered_run_script)
set(SCRIPT_OUTPUT_FILE "${SCRIPT_OUTPUT_PATH}/nodered-${OPTNS_OUTPUT}.sh")
endif()

# other necessary variables
set(EVEREST_DOCKER_DIR "${CMAKE_INSTALL_PREFIX}/${CMAKE_INSTALL_DATADIR}/everest/docker")

configure_file("${EVEREST_CONFIG_ASSET_DIR}/run_nodered_template.sh.in" ${SCRIPT_OUTPUT_FILE})

endfunction()
Loading