Copilot commented on code in PR #2216:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2216#discussion_r3672663021
##########
cmake/DockerConfig.cmake:
##########
@@ -36,6 +36,9 @@ add_custom_target(
-c DOCKER_PLATFORMS=${DOCKER_PLATFORMS}
-c DOCKER_PUSH=${DOCKER_PUSH}
-c DOCKER_TAGS=${DOCKER_TAGS}
+ -c DOCKER_USE_CONAN=${DOCKER_USE_CONAN}
+ -c DOCKER_NIFI_CONAN_USER=${DOCKER_NIFI_CONAN_USER}
+ -c DOCKER_NIFI_CONAN_PASSWORD=${DOCKER_NIFI_CONAN_PASSWORD}
Review Comment:
Passing `DOCKER_NIFI_CONAN_PASSWORD` via CMake args/command-line parameters
is unsafe because it can leak through build logs, process listings, CMake
caches, and CI output. Prefer supplying the password only via an environment
variable consumed as a BuildKit secret by `docker buildx` (and remove the
password as a CMake option / `-c` parameter), while keeping username as a
non-secret build arg if needed.
##########
docker/Dockerfile:
##########
@@ -64,11 +70,28 @@ COPY --chown=${USER}:${USER} . ${MINIFI_BASE_DIR}
USER ${USER}
-RUN mkdir ${MINIFI_BASE_DIR}/build
+RUN --mount=type=secret,id=nifi_conan_password,mode=0444 if [
"${DOCKER_USE_CONAN}" == "ON" ]; then \
+ set -e; \
+ cd ${MINIFI_BASE_DIR}/bootstrap && \
+ python3 -m venv venv && \
+ source venv/bin/activate && \
+ pip install -r requirements.txt && \
Review Comment:
Alpine images typically use `/bin/sh` (BusyBox `ash`) for `RUN`, which does
not support `source`. This will fail during Docker builds when Conan is
enabled. Use POSIX-compatible activation (`. venv/bin/activate`) or avoid shell
activation entirely (e.g., call `venv/bin/pip` and `venv/bin/python` directly).
##########
.github/workflows/ci.yml:
##########
@@ -337,6 +339,11 @@ jobs:
&& pip install -r requirements.txt \
&& python main.py --noninteractive --skip-compiler-install
--cmake-options="-DSTRICT_GSL_CHECKS=AUDIT -DCMAKE_EXPORT_COMPILE_COMMANDS=ON"
--minifi-options="${UBUNTU_CLANG_MINIFI_OPTIONS}"
working-directory: bootstrap
+ - name: Upload conan packages
+ if: always() && github.event_name == 'push' && github.ref ==
'refs/heads/main'
+ run: |
+ source venv/bin/activate && conan remote login nifi-conan && conan
upload "*" -r nifi-conan --confirm
Review Comment:
`conan upload "*"` uploads every package in the local Conan cache, which can
unintentionally publish unrelated/transitive packages and increase CI
time/network usage. It’s safer to restrict uploads to the intended
namespace/pattern (e.g., only the project’s `@minifi/*` recipes or specific
package refs) and consider uploading only on successful builds rather than
`always()`.
##########
conanfile.py:
##########
@@ -141,10 +141,15 @@ def configure(self):
setattr(self.options["aws-sdk-cpp"], "text-to-speech", False)
if self.options.enable_all or self.options.get_safe("enable_llamacpp"):
self.options["llama-cpp"].portable = self.options.portable
+ if self.options.enable_all or self.options.get_safe("enable_mqtt"):
+ self.options["paho-mqtt-c"].high_performance = True
def generate(self):
tc = CMakeToolchain(self)
tc.variables["MINIFI_DEFAULT_DEPENDENCY_SOURCE"] = "CONAN"
+
+ # Drop hardcoded install dir from Conan's generated toolchain to
detect the correct install dir for RPM package installation
+ tc.blocks.remove("output_dirs")
Review Comment:
`tc.blocks.remove("output_dirs")` can raise if the block name is not present
(or if Conan changes block naming across versions). To avoid hard failures,
guard the removal (e.g., check membership before removing) or use a supported
API to tweak output directories in a version-stable way.
##########
docker/DockerBuild.sh:
##########
@@ -223,6 +238,7 @@ if [ -n "${DISTRO_NAME}" ]; then
fi
else
if [ -n "${DOCKER_CCACHE_DUMP_LOCATION}" ]; then
+ echo docker buildx build "${BUILD_ARGS[@]}" -f "${DOCKERFILE}" --target
build -t minifi_build ..
docker buildx build "${BUILD_ARGS[@]}" -f "${DOCKERFILE}" --target build
-t minifi_build ..
Review Comment:
This debug `echo` prints the full expanded build arguments to logs, adding
noise and increasing the chance of accidentally exposing sensitive values if
new args are added later. Consider removing it or gating it behind an explicit
verbose/debug flag.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
