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
Small docker improvements and fixes #12335
Conversation
disables npm audit, fund and update-notifier for a few second startup speedup
May I join in @oplik0 ? Initially, the Host UID and Container UID are slightly different. As I show you before, I would recommend to use User And let's make respect User's favourite package manager :) |
@oplik0 is this good to merge? |
I think it should be good now. @NavyStack for now I left the user as-is and the package manager support is runtime-only, as adding it to the build would add more complexity. I'll look into it in the future though :) Also I changed the rebuild stage slightly and while it still needs to copy files it seems Also, it seems that the speedup from using the The action also no longer fetches all commits and all branches - as far as I can tell this isn't used anywhere? Maybe the metadata action once needed it, but I can't find any reference to (or reason for) this requirement - it only really needs what's on the current branch and its name. |
@oplik0 I was thinking of simply configuring a package manager-specific lock file and clearing it at the entry point. #!/bin/bash
set -e
# Function to set default values for environment variables
set_defaults() {
export CONFIG_DIR="${CONFIG_DIR:-/opt/config}"
export CONFIG="$CONFIG_DIR/config.json"
export NODEBB_INIT_VERB="${NODEBB_INIT_VERB:-install}"
export START_BUILD="${START_BUILD:-false}"
export SETUP="${SETUP:-}"
export PACKAGE_MANAGER="${PACKAGE_MANAGER:-pnpm}"
}
# Function to check if a directory exists and is writable
check_directory() {
local dir="$1"
if [ ! -d "$dir" ]; then
echo "Error: Directory $dir does not exist. Creating..."
mkdir -p "$dir" || {
echo "Error: Failed to create directory $dir"
exit 1
}
fi
if [ ! -w "$dir" ]; then
echo "Error: No write permission for directory $dir"
exit 1
fi
}
# Function to copy or link package.json and lock files based on package manager
copy_or_link_files() {
local src_dir="$1"
local dest_dir="$2"
local package_manager="$3"
local lock_file
case "$package_manager" in
yarn) lock_file="yarn.lock" ;;
npm) lock_file="package-lock.json" ;;
pnpm) lock_file="pnpm-lock.yaml" ;;
*)
echo "Unknown package manager: $package_manager"
exit 1
;;
esac
# Copy package.json and the appropriate lock file only to dest_dir if they don't exist there
if [ ! -f "$dest_dir/package.json" ]; then
cp "$src_dir/package.json" "$dest_dir/package.json"
fi
if [ ! -f "$dest_dir/$lock_file" ]; then
cp "$src_dir/$lock_file" "$dest_dir/$lock_file"
fi
# Remove unnecessary lock files in src_dir
rm -f "$src_dir/"{yarn.lock,package-lock.json,pnpm-lock.yaml}
# Symbolically link the copied files in src_dir to dest_dir
ln -fs "$dest_dir/package.json" "$src_dir/package.json"
ln -fs "$dest_dir/$lock_file" "$src_dir/$lock_file"
}
# Function to install dependencies using pnpm
install_dependencies() {
case "$PACKAGE_MANAGER" in
yarn) yarn install || {
echo "Failed to install dependencies with yarn"
exit 1
} ;;
npm) npm install || {
echo "Failed to install dependencies with npm"
exit 1
} ;;
pnpm) pnpm install || {
echo "Failed to install dependencies with pnpm"
exit 1
} ;;
*)
echo "Unknown package manager: $PACKAGE_MANAGER"
exit 1
;;
esac
}
# Function to start setup session
start_setup_session() {
local config="$1"
echo "Starting setup session"
exec /usr/src/app/nodebb setup --config="$config"
}
# Function to start forum
start_forum() {
local config="$1"
local start_build="$2"
echo "Starting forum"
if [ "$start_build" = true ]; then
echo "Build before start is enabled. Building..."
/usr/src/app/nodebb build --config="$config" || {
echo "Failed to build NodeBB. Exiting..."
exit 1
}
fi
exec "$PACKAGE_MANAGER" start --config="$config" --no-silent --no-daemon
}
# Function to start installation session
start_installation_session() {
local nodebb_init_verb="$1"
local config="$2"
echo "Config file not found at $config"
echo "Starting installation session"
exec /usr/src/app/nodebb "$nodebb_init_verb" --config="$config"
}
# Function for debugging and logging
debug_log() {
local message="$1"
echo "DEBUG: $message"
}
# Main function
main() {
set_defaults
check_directory "$CONFIG_DIR"
copy_or_link_files /usr/src/app "$CONFIG_DIR" "$PACKAGE_MANAGER"
install_dependencies
debug_log "PACKAGE_MANAGER: $PACKAGE_MANAGER"
debug_log "CONFIG location: $CONFIG"
debug_log "START_BUILD: $START_BUILD"
if [ -n "$SETUP" ]; then
start_setup_session "$CONFIG"
fi
if [ -f "$CONFIG" ]; then
start_forum "$CONFIG" "$START_BUILD"
else
start_installation_session "$NODEBB_INIT_VERB" "$CONFIG"
fi
}
# Execute main function
main "$@" Because NodeBB already includes logic to check for lock files |
This link is a docker image that reflects my preference. https://github.com/NavyStack/nodebb-docker.git |
I don't have permission to co-work on your commit. |
Fixed an update issue that could occur when updating from DockerImage v3.6.6 to v3.6.7. Overall, I'm concerned that this doesn't seem to be the right way to go for docker images. I guess it's basically because I'm trying to keep it legacy, but it's a shame I can't get too deep into it. |
Co-authored-by: NavyStack <137406386+NavyStack@users.noreply.github.com>
FROM node:lts AS git
ENV PNPM_HOME="/pnpm" \
PATH="$PNPM_HOME:$PATH" \
USER=nodebb \
UID=1001 \
GID=1001 \
TZ="Asia/Seoul"
WORKDIR /usr/src/app/
RUN groupadd --gid ${GID} ${USER} \
&& useradd --uid ${UID} --gid ${GID} --home-dir /usr/src/app/ --shell /bin/bash ${USER} \
&& chown -R ${USER}:${USER} /usr/src/app/
RUN apt-get update \
&& apt-get -y --no-install-recommends install tini
USER ${USER}
RUN git clone --recurse-submodules -j8 --depth 1 https://github.com/NodeBB/NodeBB.git .
RUN find . -mindepth 1 -maxdepth 1 -name '.*' ! -name '.' ! -name '..' -exec bash -c 'echo "Deleting {}"; rm -rf {}' \; \
&& rm -rf install/docker/entrypoint.sh \
&& rm -rf docker-compose.yml \
&& rm -rf Dockerfile
## && sed -i 's|"\*/jquery":|"jquery":|g' install/package.json
FROM node:lts AS node_modules_touch
ENV NODE_ENV=production \
PNPM_HOME="/pnpm" \
PATH="$PNPM_HOME:$PATH" \
USER=nodebb \
UID=1001 \
GID=1001 \
TZ="Asia/Seoul"
WORKDIR /usr/src/app/
RUN corepack enable \
&& groupadd --gid ${GID} ${USER} \
&& useradd --uid ${UID} --gid ${GID} --home-dir /usr/src/app/ --shell /bin/bash ${USER} \
&& chown -R ${USER}:${USER} /usr/src/app/
COPY --from=git --chown=${USER}:${USER} /usr/src/app/install/package.json /usr/src/app/
USER ${USER}
RUN --mount=type=cache,id=pnpm,target=/pnpm/store \
npm install \
@nodebb/nodebb-plugin-reactions \
nodebb-plugin-adsense \
nodebb-plugin-extended-markdown \
nodebb-plugin-meilisearch \
nodebb-plugin-question-and-answer \
nodebb-plugin-sso-github \
&& npm install --package-lock-only --omit=dev
## pnpm import \
## && pnpm install --prod --frozen-lockfile
FROM node:lts-slim AS final
ENV NODE_ENV=production \
DAEMON=false \
SILENT=false \
PNPM_HOME="/pnpm" \
PATH="$PNPM_HOME:$PATH" \
USER=nodebb \
UID=1001 \
GID=1001 \
TZ="Asia/Seoul"
WORKDIR /usr/src/app/
RUN corepack enable \
&& groupadd --gid ${GID} ${USER} \
&& useradd --uid ${UID} --gid ${GID} --home-dir /usr/src/app/ --shell /bin/bash ${USER} \
&& mkdir -p /usr/src/app/logs/ /opt/config/ \
&& chown -R ${USER}:${USER} /usr/src/app/ /opt/config/
COPY --from=node_modules_touch --chown=${USER}:${USER} /usr/src/app/ /usr/src/app/
COPY --from=git --chown=${USER}:${USER} /usr/src/app/ /usr/src/app/
COPY --from=git --chown=${USER}:${USER} /usr/src/app/install/docker/setup.json /usr/src/app/setup.json
COPY --from=git --chown=${USER}:${USER} /usr/bin/tini /usr/bin/tini
COPY --chown=${USER}:${USER} docker-entrypoint.sh /usr/local/bin/
USER ${USER}
EXPOSE 4567
VOLUME ["/usr/src/app/node_modules", "/usr/src/app/build", "/usr/src/app/public/uploads", "/opt/config/"]
ENTRYPOINT ["tini", "--", "docker-entrypoint.sh"]
## ENTRYPOINT ["tini", "--"]
## CMD ["start.sh"]
|
@NavyStack Sent an invite :) |
@oplik0 It's an honour to work with you. |
Biggest change that matters. changed USER
Considering the above, I made a drastic change.
Running commands in a Docker image? That's not recommended! If you need to make modifications to the container, you should rebuild the image instead. |
@NavyStack @oplik0 Any news on this PR? it's been a couple of months and I would really like to see it merged so that it can resolve a couple of issues with my version of NodeBB on Docker. |
@NavyStack made small changes and I think it looks fine now, anything you'd still want to add/change for this PR? I agree that modifying a running container is far from perfect, but changing that would require more fundamental changes to the docker setup and maybe some improvements to NodeBB. So it'll probably have to wait for at least 4.0 (or even later major release since AP is the priority for 4.0) |
@oplik0 It looks good to me. @xSkeletor Thanks. |
This PR addresses some smaller issues with the current docker setup:
npm fund
and update checks for npm were also disabled for similar reason, though their performance impact is much smallernodebb upgrade
is now ran when the container is updatedinstall/package.json
build
toupgrade
and leaving users to force build manually.defaults
key that's read into the default for questions in all database driver, so it's the only "nodebb code" change here.gitkeep
s in.docker
uid
of 1000 on host, but I'm not sure if there is any fix there