Onboard valkey-container to distgen-generated sources#65
Merged
Conversation
📝 WalkthroughWalkthroughThis PR removes selected v7 artifacts, adds a manifest/multispec-driven generation pipeline for version 8, parameterizes source Dockerfile templates, introduces container runtime init and helper scripts, and updates v8 Dockerfile labels and repository symlinks. ChangesVersion 7 Removal and Test Cleanup
Template-Based Build System and Image Generation
Container Runtime and Initialization Infrastructure
Version 8 Dockerfile Label Hardcoding and Symlinks
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Author
|
[test] |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
7/Dockerfile.fedora (1)
53-66:⚠️ Potential issue | 🟠 Major | ⚖️ Poor tradeoffRemove
--nogpgcheckto enforce package signature verification.Line 58 disables GPG signature verification with
--nogpgcheck, which allows installation of unsigned or tampered packages. This exposes the container to supply-chain attacks if the repository is compromised or a man-in-the-middle attack occurs during package download.🔒 Recommended fix
- dnf install -y --setopt=tsflags=nodocs --nogpgcheck $INSTALL_PKGS && \ + dnf install -y --setopt=tsflags=nodocs $INSTALL_PKGS && \If GPG keys are missing, import them explicitly before installation rather than disabling verification.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@7/Dockerfile.fedora` around lines 53 - 66, The RUN step that installs $INSTALL_PKGS currently uses --nogpgcheck (dnf install -y --setopt=tsflags=nodocs --nogpgcheck $INSTALL_PKGS); remove the --nogpgcheck flag and instead import and trust the repository/package GPG keys before the install (e.g., with rpm --import or by placing the repo's GPG key in the system keyring), ensuring package signature verification is enabled during the dnf install in this RUN block; keep the rest of the steps (rpm -V, dnf clean, version check using valkey-server --version and the mkdir/chown/chmod/id checks) unchanged.Source: Linters/SAST tools
8/Dockerfile.fedora (1)
53-66:⚠️ Potential issue | 🟠 Major | ⚖️ Poor tradeoffRemove
--nogpgcheckto enforce package signature verification.Line 58 disables GPG signature verification with
--nogpgcheck, which allows installation of unsigned or tampered packages. This exposes the container to supply-chain attacks if the repository is compromised or a man-in-the-middle attack occurs during package download.🔒 Recommended fix
- dnf install -y --setopt=tsflags=nodocs --nogpgcheck $INSTALL_PKGS && \ + dnf install -y --setopt=tsflags=nodocs $INSTALL_PKGS && \If GPG keys are missing, import them explicitly before installation rather than disabling verification.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@8/Dockerfile.fedora` around lines 53 - 66, The dnf install line that uses "--nogpgcheck" must be changed to enforce GPG verification: remove the "--nogpgcheck" flag from the dnf install command that installs $INSTALL_PKGS and instead import the repository/package signing keys before installation (e.g., using rpm --import or dnf keyring/gnupg import for the repo) so package signatures can be validated; update the Dockerfile sequence around INSTALL_PKGS and the "dnf install -y --setopt=tsflags=nodocs" invocation to ensure keys are imported prior to invoking dnf, failing the build if keys are missing.
🧹 Nitpick comments (4)
src/Dockerfile (1)
68-71: ⚡ Quick winRemove duplicate ENV declaration.
Lines 69-71 redefine
CONTAINER_SCRIPTS_PATH,VALKEY_PREFIX, andVALKEY_CONFwhich were already set at lines 39-44. The duplicate declaration is unnecessary.♻️ Remove the duplicate ENV block
-# Get prefix path and path to scripts rather than hard-code them in scripts -ENV CONTAINER_SCRIPTS_PATH=/usr/share/container-scripts/valkey \ - VALKEY_PREFIX=/usr \ - VALKEY_CONF=/etc/valkey/valkey.conf - COPY root /🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Dockerfile` around lines 68 - 71, Remove the duplicate ENV block that redefines CONTAINER_SCRIPTS_PATH, VALKEY_PREFIX, and VALKEY_CONF; locate the second ENV declaration that sets CONTAINER_SCRIPTS_PATH=/usr/share/container-scripts/valkey, VALKEY_PREFIX=/usr, and VALKEY_CONF=/etc/valkey/valkey.conf and delete it so those variables are only declared once (keep the original ENV at the earlier lines and ensure no other duplicate ENV entries remain).src/root/usr/share/container-scripts/valkey/validate-variables.sh (1)
1-6: 💤 Low valueUse POSIX-compliant string comparison.
Line 2 uses
[ $# == 1 ]with the==operator, which is not POSIX-compliant when using the[command (though it works in bash). For maximum portability and consistency with strict shell practices, use=instead.♻️ Proposed refactor
function usage() { - [ $# == 1 ] && echo "error: $1" + [ $# = 1 ] && echo "error: $1" echo "You can specify the following environment variables:" echo " VALKEY_PASSWORD (regex: '$valkey_password_regex')" exit 1 }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/root/usr/share/container-scripts/valkey/validate-variables.sh` around lines 1 - 6, The test in function usage() uses the non-POSIX operator "==" when checking positional parameters; change the conditional to use the POSIX-compliant "=" operator (i.e., replace `[ $# == 1 ]` with `[ $# = 1 ]`) so the script remains portable; keep the rest of the usage() behavior (echo messages referencing valkey_password_regex and exit 1) unchanged.src/root/usr/share/container-scripts/valkey/helpers.sh (2)
17-23: ⚡ Quick winAvoid toggling
set -earound command sequences.Temporarily disabling error-exit mode with
set +eand re-enabling it withset -eis fragile. If an unexpected exit occurs between lines 17 and 23 (e.g., from a signal),set -emight not be restored. Additionally,mountandlsfailures in a debug logging function are acceptable and don't require disabling strict mode for the entire block.♻️ Proposed refactor using subshells or conditional execution
function log_volume_info { CONTAINER_DEBUG=${CONTAINER_DEBUG:-} if [[ "${CONTAINER_DEBUG,,}" != "true" ]]; then return fi log_info "Volume info for $@:" - set +e - log_and_run mount + log_and_run mount || true while [ $# -gt 0 ]; do - log_and_run ls -alZ $1 + log_and_run ls -alZ $1 || true shift done - set -e }This approach allows individual commands to fail without disabling error mode for the entire function.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/root/usr/share/container-scripts/valkey/helpers.sh` around lines 17 - 23, The debug block currently disables errexit with "set +e" and re-enables it with "set -e", which is fragile; instead, avoid toggling shell state by allowing individual commands to fail (e.g., run mount and each ls under the existing log_and_run wrapper in a subshell or append "|| true" per invocation). Update the sequence around log_and_run mount and the while loop that calls log_and_run ls -alZ $1 so that you remove "set +e" and "set -e" and ensure failures of mount or ls do not abort the script by making each call tolerant of errors (use a subshell, conditional execution, or explicit "|| true" on the log_and_run invocations) while keeping the rest of the function under strict mode.
1-3: ⚡ Quick winReplace deprecated backtick syntax with modern command substitution.
Line 2 uses backticks for command substitution, which is deprecated in modern shell scripting. The
$(...)syntax is preferred because it's more readable, nests better, and has clearer quoting rules.♻️ Proposed refactor
function log_info { - echo "---> `date +%T` $@" + echo "---> $(date +%T) $@" }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/root/usr/share/container-scripts/valkey/helpers.sh` around lines 1 - 3, In function log_info replace the deprecated backtick command substitution with the modern $(...) form: update the echo invocation inside log_info (the function named log_info) to use $(date +%T) instead of `date +%T`, preserving spacing and the $@ argument so output formatting remains identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/root/usr/bin/run-valkey`:
- Line 23: The comment currently says "Restart the Redis server with public IP
bindings" but this script pertains to Valkey; update the comment in run-valkey
(the restart/redis-related comment) to reference restarting the Valkey
container/service (e.g., "Restart the Valkey container with public IP bindings"
or similar) so it accurately describes the operation performed by the script and
avoids mentioning Redis.
- Line 9: The inline comment in the run-valkey script incorrectly references
Redis; update the comment text from "Process the Redis configuration files" to
reference Valkey (e.g., "Process the Valkey configuration files") so it
accurately describes the script's purpose; edit the comment line in the
run-valkey file where that string appears to make this one-word replacement.
- Line 25: run-valkey currently calls log_volume_info "${VALKEY_DATADIR}" but
may not have VALKEY_DATADIR in scope; ensure the script sources
src/root/usr/share/container-scripts/valkey/common.sh (which exports
VALKEY_DATADIR) before the log_volume_info invocation. Modify run-valkey to
source the common.sh at the top (or at least before any use of VALKEY_DATADIR),
so the exported VALKEY_DATADIR variable is defined and available to
log_volume_info and any subsequent logic.
In `@src/root/usr/libexec/container-setup`:
- Around line 3-4: Move the strict-mode directive so failures while sourcing
dependencies are caught: place the set -eu line at the top of the script before
the source ${CONTAINER_SCRIPTS_PATH}/common.sh invocation (so
CONTAINER_SCRIPTS_PATH and the common.sh file are validated under strict mode);
ensure no commands that rely on non-set variables appear before set -eu.
- Line 8: The container-setup script currently runs restorecon -R "${HOME}"
"${VALKEY_CONF}" unconditionally which will fail on systems without SELinux or
without the restorecon binary; wrap that call in a guard that first checks for
the restorecon command and for SELinux being enabled (e.g., using command -v
restorecon and either selinuxenabled or getenforce) and only call restorecon -R
"${HOME}" "${VALKEY_CONF}" when both checks pass; update the block around the
restorecon invocation in container-setup to perform these runtime checks and
skip the call silently when restorecon/SELinux are not present.
In `@src/root/usr/share/container-scripts/valkey/common.sh`:
- Around line 28-34: The setup_bind_address function appends a bind directive
unconditionally causing duplicates; modify it to be idempotent by checking
VALKEY_CONF for an existing "bind" line (or a bind for the same interface)
before writing: if a bind line exists, replace or update that line to "bind
$BIND_ADDRESS", otherwise append the new directive. Use the existing symbols
VALKEY_CONF, BIND_ADDRESS and the setup_bind_address function to locate where to
perform the check/replace so subsequent calls won't produce duplicate bind
entries.
- Around line 20-26: The clear_config function currently unconditionally flips
"protected-mode yes" to "protected-mode no" which creates a security risk;
change clear_config so it only disables protected-mode when it's safe by
checking environment variables (e.g., VALKEY_PASSWORD and BIND_ADDRESS) before
applying the sed replacement on ${VALKEY_CONF}, otherwise leave protected-mode
unchanged (or fail/require VALKEY_PASSWORD). Update the logic around the
protected-mode sed command in clear_config to conditionally run the
"/^protected-mode/s/yes/no/" edit based on those checks and ensure any path that
disables protected-mode validates both authentication and bind configuration
first.
In `@src/root/usr/share/container-scripts/valkey/post-init.sh`:
- Around line 8-9: Move the shell strict-mode invocation so it runs before
sourcing dependencies: place the `set -eu` line before the `source
"${CONTAINER_SCRIPTS_PATH}/common.sh"` invocation so errors (like an unset
`CONTAINER_SCRIPTS_PATH` or missing `common.sh`) are caught immediately; update
the `post-init.sh` startup sequence to call `set -eu` first, then `source
"${CONTAINER_SCRIPTS_PATH}/common.sh"`.
In `@src/root/usr/share/container-scripts/valkey/README.md`:
- Line 70: Fix the typo in the README notice by replacing "mouting" with
"mounting" in the sentence that begins "**Notice: When mouting a directory from
the host into the container, ensure that the mounted"; locate that exact notice
text in src/root/usr/share/container-scripts/valkey/README.md and update the
word to "mounting" so the sentence reads correctly.
- Around line 86-87: Replace the copy-paste text that incorrectly references
"Python environment Dockerfiles" and the grammar "another versions" in the
README: update the sentence to correctly describe this as the Valkey container
image repo and refer to "other versions" of Dockerfiles, e.g. "In this
repository you can also find other versions of container Dockerfiles; CentOS
Stream 10 is Dockerfile.c10s and Fedora is Dockerfile.fedora" (locate and edit
the paragraph containing "Python environment Dockerfiles" and "another
versions").
---
Outside diff comments:
In `@7/Dockerfile.fedora`:
- Around line 53-66: The RUN step that installs $INSTALL_PKGS currently uses
--nogpgcheck (dnf install -y --setopt=tsflags=nodocs --nogpgcheck
$INSTALL_PKGS); remove the --nogpgcheck flag and instead import and trust the
repository/package GPG keys before the install (e.g., with rpm --import or by
placing the repo's GPG key in the system keyring), ensuring package signature
verification is enabled during the dnf install in this RUN block; keep the rest
of the steps (rpm -V, dnf clean, version check using valkey-server --version and
the mkdir/chown/chmod/id checks) unchanged.
In `@8/Dockerfile.fedora`:
- Around line 53-66: The dnf install line that uses "--nogpgcheck" must be
changed to enforce GPG verification: remove the "--nogpgcheck" flag from the dnf
install command that installs $INSTALL_PKGS and instead import the
repository/package signing keys before installation (e.g., using rpm --import or
dnf keyring/gnupg import for the repo) so package signatures can be validated;
update the Dockerfile sequence around INSTALL_PKGS and the "dnf install -y
--setopt=tsflags=nodocs" invocation to ensure keys are imported prior to
invoking dnf, failing the build if keys are missing.
---
Nitpick comments:
In `@src/Dockerfile`:
- Around line 68-71: Remove the duplicate ENV block that redefines
CONTAINER_SCRIPTS_PATH, VALKEY_PREFIX, and VALKEY_CONF; locate the second ENV
declaration that sets
CONTAINER_SCRIPTS_PATH=/usr/share/container-scripts/valkey, VALKEY_PREFIX=/usr,
and VALKEY_CONF=/etc/valkey/valkey.conf and delete it so those variables are
only declared once (keep the original ENV at the earlier lines and ensure no
other duplicate ENV entries remain).
In `@src/root/usr/share/container-scripts/valkey/helpers.sh`:
- Around line 17-23: The debug block currently disables errexit with "set +e"
and re-enables it with "set -e", which is fragile; instead, avoid toggling shell
state by allowing individual commands to fail (e.g., run mount and each ls under
the existing log_and_run wrapper in a subshell or append "|| true" per
invocation). Update the sequence around log_and_run mount and the while loop
that calls log_and_run ls -alZ $1 so that you remove "set +e" and "set -e" and
ensure failures of mount or ls do not abort the script by making each call
tolerant of errors (use a subshell, conditional execution, or explicit "|| true"
on the log_and_run invocations) while keeping the rest of the function under
strict mode.
- Around line 1-3: In function log_info replace the deprecated backtick command
substitution with the modern $(...) form: update the echo invocation inside
log_info (the function named log_info) to use $(date +%T) instead of `date +%T`,
preserving spacing and the $@ argument so output formatting remains identical.
In `@src/root/usr/share/container-scripts/valkey/validate-variables.sh`:
- Around line 1-6: The test in function usage() uses the non-POSIX operator "=="
when checking positional parameters; change the conditional to use the
POSIX-compliant "=" operator (i.e., replace `[ $# == 1 ]` with `[ $# = 1 ]`) so
the script remains portable; keep the rest of the usage() behavior (echo
messages referencing valkey_password_regex and exit 1) unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 48a5a4a7-1c9c-4361-b9e4-a148c8c5f060
📒 Files selected for processing (46)
7/Dockerfile.c10s7/Dockerfile.fedora7/Dockerfile.rhel107/README.md7/README.md7/test7/test/check_imagestreams.py7/test/examples/valkey-ephemeral-template.json7/test/examples/valkey-persistent-template.json7/test/imagestreams/imagestreams.yaml7/test/imagestreams/valkey-centos.json7/test/run7/test/run-openshift-pytest7/test/run-openshift-remote-cluster7/test/show_all_imagestreams.py7/test/test-lib-openshift.sh7/test/test-lib-remote-openshift.sh7/test/test-lib-valkey.sh7/test/test-lib.sh7/test/test-openshift.yaml7/test/test_valkey_imagestream.py7/test/test_valkey_imagestream_template.py7/test/test_valkey_latest_imagestreams.py7/test/test_valkey_template.py7/test/valkey-persistent-template.json8/Dockerfile.c10s8/Dockerfile.fedora8/Dockerfile.rhel108/Dockerfile.rhel98/testMakefilemanifest.ymlspecs/multispec.ymlsrc/Dockerfilesrc/Dockerfile.fedorasrc/root/usr/bin/container-entrypointsrc/root/usr/bin/run-valkeysrc/root/usr/bin/usagesrc/root/usr/libexec/container-setupsrc/root/usr/share/container-scripts/valkey/README.mdsrc/root/usr/share/container-scripts/valkey/base.conf.templatesrc/root/usr/share/container-scripts/valkey/common.shsrc/root/usr/share/container-scripts/valkey/helpers.shsrc/root/usr/share/container-scripts/valkey/password.conf.templatesrc/root/usr/share/container-scripts/valkey/post-init.shsrc/root/usr/share/container-scripts/valkey/validate-variables.sh
💤 Files with no reviewable changes (19)
- 7/test/test-openshift.yaml
- 7/test/valkey-persistent-template.json
- 7/test/imagestreams/imagestreams.yaml
- 7/test/run-openshift-pytest
- 7/test/run
- 7/test/run-openshift-remote-cluster
- 7/test/imagestreams/valkey-centos.json
- 7/test/examples/valkey-persistent-template.json
- 7/test/check_imagestreams.py
- 7/test/test_valkey_imagestream_template.py
- 7/test/test-lib-valkey.sh
- 7/test/test_valkey_template.py
- 7/test/test-lib-openshift.sh
- 7/test/examples/valkey-ephemeral-template.json
- 7/test/test-lib.sh
- 7/test/test_valkey_latest_imagestreams.py
- 7/test/show_all_imagestreams.py
- 7/test/test-lib-remote-openshift.sh
- 7/test/test_valkey_imagestream.py
|
|
||
| [ -f ${CONTAINER_SCRIPTS_PATH}/validate-variables.sh ] && source ${CONTAINER_SCRIPTS_PATH}/validate-variables.sh | ||
|
|
||
| # Process the Redis configuration files |
There was a problem hiding this comment.
Update comment to reference Valkey instead of Redis.
The comment says "Process the Redis configuration files" but this is a Valkey container.
📝 Proposed fix
-# Process the Redis configuration files
+# Process the Valkey configuration files
log_info 'Processing Valkey configuration files ...'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # Process the Redis configuration files | |
| # Process the Valkey configuration files | |
| log_info 'Processing Valkey configuration files ...' |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/root/usr/bin/run-valkey` at line 9, The inline comment in the run-valkey
script incorrectly references Redis; update the comment text from "Process the
Redis configuration files" to reference Valkey (e.g., "Process the Valkey
configuration files") so it accurately describes the script's purpose; edit the
comment line in the run-valkey file where that string appears to make this
one-word replacement.
| source ${CONTAINER_SCRIPTS_PATH}/post-init.sh | ||
| fi | ||
|
|
||
| # Restart the Redis server with public IP bindings |
There was a problem hiding this comment.
Update comment to reference Valkey instead of Redis.
The comment says "Restart the Redis server" but this is a Valkey container.
📝 Proposed fix
-# Restart the Redis server with public IP bindings
+# Start the Valkey server with public IP bindings
unset_env_vars📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # Restart the Redis server with public IP bindings | |
| # Start the Valkey server with public IP bindings |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/root/usr/bin/run-valkey` at line 23, The comment currently says "Restart
the Redis server with public IP bindings" but this script pertains to Valkey;
update the comment in run-valkey (the restart/redis-related comment) to
reference restarting the Valkey container/service (e.g., "Restart the Valkey
container with public IP bindings" or similar) so it accurately describes the
operation performed by the script and avoids mentioning Redis.
|
|
||
| # Restart the Redis server with public IP bindings | ||
| unset_env_vars | ||
| log_volume_info "${VALKEY_DATADIR}" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Search for VALKEY_DATADIR definition across the codebase
echo "Searching for VALKEY_DATADIR definition:"
rg -n "VALKEY_DATADIR\s*=" --type sh
echo -e "\nSearching for VALKEY_DATADIR usage:"
rg -n "VALKEY_DATADIR" --type sh -C2Repository: sclorg/valkey-container
Length of output: 1804
Ensure VALKEY_DATADIR is in-scope (sourced) in run-valkey.
VALKEY_DATADIR is exported in src/root/usr/share/container-scripts/valkey/common.sh (export VALKEY_DATADIR=/var/lib/valkey/data). Confirm src/root/usr/bin/run-valkey sources common.sh before log_volume_info "${VALKEY_DATADIR}".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/root/usr/bin/run-valkey` at line 25, run-valkey currently calls
log_volume_info "${VALKEY_DATADIR}" but may not have VALKEY_DATADIR in scope;
ensure the script sources src/root/usr/share/container-scripts/valkey/common.sh
(which exports VALKEY_DATADIR) before the log_volume_info invocation. Modify
run-valkey to source the common.sh at the top (or at least before any use of
VALKEY_DATADIR), so the exported VALKEY_DATADIR variable is defined and
available to log_volume_info and any subsequent logic.
Comment on lines
+3
to
+4
| source ${CONTAINER_SCRIPTS_PATH}/common.sh | ||
| set -eu |
There was a problem hiding this comment.
Enable strict mode before sourcing dependencies.
The set -eu directive is placed after sourcing common.sh, which means errors or unset variables during the source operation will not be caught. If CONTAINER_SCRIPTS_PATH is unset or the file doesn't exist, the script will continue silently or fail with a less helpful error.
🛡️ Proposed fix to enable strict mode first
+set -eu
+
source ${CONTAINER_SCRIPTS_PATH}/common.sh
-set -eu📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| source ${CONTAINER_SCRIPTS_PATH}/common.sh | |
| set -eu | |
| set -eu | |
| source ${CONTAINER_SCRIPTS_PATH}/common.sh |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/root/usr/libexec/container-setup` around lines 3 - 4, Move the
strict-mode directive so failures while sourcing dependencies are caught: place
the set -eu line at the top of the script before the source
${CONTAINER_SCRIPTS_PATH}/common.sh invocation (so CONTAINER_SCRIPTS_PATH and
the common.sh file are validated under strict mode); ensure no commands that
rely on non-set variables appear before set -eu.
|
|
||
| # setup directory for data | ||
| chown -R valkey:0 "${HOME}" "${VALKEY_CONF}" | ||
| restorecon -R "${HOME}" "${VALKEY_CONF}" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check for SELinux-aware patterns in container scripts
# Search for restorecon usage and conditional SELinux checks
rg -n 'restorecon|selinuxenabled|getenforce' --type sh
# Look for command existence checks
rg -n 'command -v restorecon|which restorecon|type restorecon' --type shRepository: sclorg/valkey-container
Length of output: 49
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Inspect the relevant file around the reported line
echo "=== container-setup (context around restorecon) ==="
nl -ba src/root/usr/libexec/container-setup | sed -n '1,120p'
# 2) Search repo-wide for SELinux/restorecon usage (not restricted by file type)
echo "=== restorecon occurrences ==="
rg -n "restorecon" .
echo "=== SELinux guard/utility occurrences ==="
rg -n "selinuxenabled|getenforce" .
# 3) Search for restorecon existence checks (repo-wide)
echo "=== restorecon availability checks ==="
rg -n "(command -v|which|type)[[:space:]]+restorecon|restorecon[[:space:]]+.*\?|\btest\b.*\b-x\b.*restorecon" .Repository: sclorg/valkey-container
Length of output: 180
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show first ~80 lines with line numbers (nl may not exist in sandbox)
echo "=== container-setup (first 80 lines) ==="
awk '{printf "%5d %s\n", NR, $0}' src/root/usr/libexec/container-setup | sed -n '1,80p'
echo "=== restorecon occurrences (files) ==="
rg -l "restorecon" .
echo "=== restorecon occurrences (snippets) ==="
rg -n "restorecon" . -S
echo "=== SELinux guard occurrences ==="
rg -n "selinuxenabled|getenforce" . -S
echo "=== restorecon availability checks (command/type/which) ==="
rg -n "(command[[:space:]]+-v|which|type)[[:space:]]+restorecon" . -SRepository: sclorg/valkey-container
Length of output: 1304
🏁 Script executed:
#!/bin/bash
set +euo pipefail
echo "=== SELinux guard occurrences ==="
rg -n "selinuxenabled|getenforce" . -S
echo "exit=$?"
echo "=== restorecon availability checks (command/type/which) ==="
rg -n "(command[[:space:]]+-v|which|type)[[:space:]]+restorecon" . -S
echo "exit=$?"
echo "=== all restorecon occurrences with line numbers ==="
rg -n "restorecon" . -SRepository: sclorg/valkey-container
Length of output: 470
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== locate common.sh ==="
fd -a "common.sh" . || true
echo "=== search common.sh for SELinux/restorecon ==="
fd -a "common.sh" . -x sh -c 'echo "--- $1 ---"; rg -n "selinuxenabled|getenforce|restorecon" "$1" -S || true' shRepository: sclorg/valkey-container
Length of output: 594
🏁 Script executed:
#!/bin/bash
set +euo pipefail
COMMONS=(
"src/root/usr/share/container-scripts/valkey/common.sh"
"7/root/usr/share/container-scripts/valkey/common.sh"
"8/root/usr/share/container-scripts/valkey/common.sh"
)
for f in "${COMMONS[@]}"; do
echo "=== $f: SELinux/restorecon matches ==="
rg -n "selinuxenabled|getenforce|restorecon" "$f" -S || true
done
echo "=== container-setup: common.sh source line check ==="
sed -n '1,20p' src/root/usr/libexec/container-setupRepository: sclorg/valkey-container
Length of output: 998
Guard restorecon so non-SELinux environments don’t fail startup
src/root/usr/libexec/container-setup unconditionally runs restorecon -R "${HOME}" "${VALKEY_CONF}" while using set -eu, and there are no repo-wide checks/guards for SELinux (selinuxenabled/getenforce) or for restorecon availability (command -v/which/type). This will fail on images/systems where restorecon (or SELinux) isn’t present.
Current call site
restorecon -R "${HOME}" "${VALKEY_CONF}"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/root/usr/libexec/container-setup` at line 8, The container-setup script
currently runs restorecon -R "${HOME}" "${VALKEY_CONF}" unconditionally which
will fail on systems without SELinux or without the restorecon binary; wrap that
call in a guard that first checks for the restorecon command and for SELinux
being enabled (e.g., using command -v restorecon and either selinuxenabled or
getenforce) and only call restorecon -R "${HOME}" "${VALKEY_CONF}" when both
checks pass; update the block around the restorecon invocation in
container-setup to perform these runtime checks and skip the call silently when
restorecon/SELinux are not present.
Comment on lines
+20
to
+26
| function clear_config() { | ||
| sed -e "s/^bind/#bind/" \ | ||
| -e "s/^logfile/#logfile/" \ | ||
| -e "s/^dir /#dir /" \ | ||
| -e "/^protected-mode/s/yes/no/" \ | ||
| -i "${VALKEY_CONF}" | ||
| } |
There was a problem hiding this comment.
Disabling protected-mode creates a security risk.
Line 24 unconditionally changes protected-mode yes to protected-mode no. This allows Valkey to accept connections from any address when bind is not set, even without authentication. If a container is started without VALKEY_PASSWORD and BIND_ADDRESS, the Valkey instance will be exposed without protection.
🔐 Suggested mitigation
Consider one of these approaches:
- Keep protected-mode enabled by default and only disable it when both password and bind are configured:
function clear_config() {
sed -e "s/^bind/#bind/" \
-e "s/^logfile/#logfile/" \
-e "s/^dir /#dir /" \
- -e "/^protected-mode/s/yes/no/" \
-i "${VALKEY_CONF}"
}Then disable it conditionally only after validation.
-
Require VALKEY_PASSWORD when protected-mode is disabled.
-
Document the security implications prominently in the README if this is intentional for container environments.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function clear_config() { | |
| sed -e "s/^bind/#bind/" \ | |
| -e "s/^logfile/#logfile/" \ | |
| -e "s/^dir /#dir /" \ | |
| -e "/^protected-mode/s/yes/no/" \ | |
| -i "${VALKEY_CONF}" | |
| } | |
| function clear_config() { | |
| sed -e "s/^bind/#bind/" \ | |
| -e "s/^logfile/#logfile/" \ | |
| -e "s/^dir /#dir /" \ | |
| -i "${VALKEY_CONF}" | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/root/usr/share/container-scripts/valkey/common.sh` around lines 20 - 26,
The clear_config function currently unconditionally flips "protected-mode yes"
to "protected-mode no" which creates a security risk; change clear_config so it
only disables protected-mode when it's safe by checking environment variables
(e.g., VALKEY_PASSWORD and BIND_ADDRESS) before applying the sed replacement on
${VALKEY_CONF}, otherwise leave protected-mode unchanged (or fail/require
VALKEY_PASSWORD). Update the logic around the protected-mode sed command in
clear_config to conditionally run the "/^protected-mode/s/yes/no/" edit based on
those checks and ensure any path that disables protected-mode validates both
authentication and bind configuration first.
Comment on lines
+28
to
+34
| function setup_bind_address() { | ||
| # Function sets bind | ||
| if [[ -v BIND_ADDRESS ]]; then | ||
| log_info "Configuring valkey to listen on $BIND_ADDRESS only ..." | ||
| echo "bind $BIND_ADDRESS" >> "${VALKEY_CONF}" | ||
| fi | ||
| } |
There was a problem hiding this comment.
Prevent duplicate bind directives in configuration.
The function appends bind $BIND_ADDRESS to the config file without checking if a bind directive already exists. If this function is called multiple times (e.g., during restarts or in different initialization paths), duplicate bind directives will accumulate, and Valkey may use only the last one or fail to start.
🔧 Proposed fix to ensure idempotency
function setup_bind_address() {
# Function sets bind
if [[ -v BIND_ADDRESS ]]; then
log_info "Configuring valkey to listen on $BIND_ADDRESS only ..."
+ # Remove any existing bind directives first
+ sed -i '/^bind /d' "${VALKEY_CONF}"
echo "bind $BIND_ADDRESS" >> "${VALKEY_CONF}"
fi
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function setup_bind_address() { | |
| # Function sets bind | |
| if [[ -v BIND_ADDRESS ]]; then | |
| log_info "Configuring valkey to listen on $BIND_ADDRESS only ..." | |
| echo "bind $BIND_ADDRESS" >> "${VALKEY_CONF}" | |
| fi | |
| } | |
| function setup_bind_address() { | |
| # Function sets bind | |
| if [[ -v BIND_ADDRESS ]]; then | |
| log_info "Configuring valkey to listen on $BIND_ADDRESS only ..." | |
| # Remove any existing bind directives first | |
| sed -i '/^bind /d' "${VALKEY_CONF}" | |
| echo "bind $BIND_ADDRESS" >> "${VALKEY_CONF}" | |
| fi | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/root/usr/share/container-scripts/valkey/common.sh` around lines 28 - 34,
The setup_bind_address function appends a bind directive unconditionally causing
duplicates; modify it to be idempotent by checking VALKEY_CONF for an existing
"bind" line (or a bind for the same interface) before writing: if a bind line
exists, replace or update that line to "bind $BIND_ADDRESS", otherwise append
the new directive. Use the existing symbols VALKEY_CONF, BIND_ADDRESS and the
setup_bind_address function to locate where to perform the check/replace so
subsequent calls won't produce duplicate bind entries.
Comment on lines
+8
to
+9
| source "${CONTAINER_SCRIPTS_PATH}/common.sh" | ||
| set -eu |
There was a problem hiding this comment.
Enable strict mode before sourcing dependencies.
Same issue as in container-setup: set -eu should be placed before the source command to catch errors during sourcing. If CONTAINER_SCRIPTS_PATH is unset or the file is missing, the error will be less clear.
🛡️ Proposed fix
+set -eu
+
source "${CONTAINER_SCRIPTS_PATH}/common.sh"
-set -eu📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| source "${CONTAINER_SCRIPTS_PATH}/common.sh" | |
| set -eu | |
| set -eu | |
| source "${CONTAINER_SCRIPTS_PATH}/common.sh" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/root/usr/share/container-scripts/valkey/post-init.sh` around lines 8 - 9,
Move the shell strict-mode invocation so it runs before sourcing dependencies:
place the `set -eu` line before the `source
"${CONTAINER_SCRIPTS_PATH}/common.sh"` invocation so errors (like an unset
`CONTAINER_SCRIPTS_PATH` or missing `common.sh`) are caught immediately; update
the `post-init.sh` startup sequence to call `set -eu` first, then `source
"${CONTAINER_SCRIPTS_PATH}/common.sh"`.
| Valkey data directory | ||
|
|
||
|
|
||
| **Notice: When mouting a directory from the host into the container, ensure that the mounted |
There was a problem hiding this comment.
Fix typo in the notice.
"mouting" should be "mounting".
📝 Proposed fix
-**Notice: When mouting a directory from the host into the container, ensure that the mounted
+**Notice: When mounting a directory from the host into the container, ensure that the mounted📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| **Notice: When mouting a directory from the host into the container, ensure that the mounted | |
| **Notice: When mounting a directory from the host into the container, ensure that the mounted |
🧰 Tools
🪛 LanguageTool
[grammar] ~70-~70: Ensure spelling is correct
Context: ... Valkey data directory **Notice: When mouting a directory from the host into the cont...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/root/usr/share/container-scripts/valkey/README.md` at line 70, Fix the
typo in the README notice by replacing "mouting" with "mounting" in the sentence
that begins "**Notice: When mouting a directory from the host into the
container, ensure that the mounted"; locate that exact notice text in
src/root/usr/share/container-scripts/valkey/README.md and update the word to
"mounting" so the sentence reads correctly.
Comment on lines
+86
to
+87
| In that repository you also can find another versions of Python environment Dockerfiles. | ||
| Dockerfile for CentOS Stream 10 it's `Dockerfile.c10s` and the Fedora Dockerfile is called Dockerfile.fedora. |
There was a problem hiding this comment.
Correct copy-paste error mentioning Python.
Lines 86-87 reference "Python environment Dockerfiles," but this is a Valkey container image repository. This appears to be a copy-paste artifact from a Python container template. Additionally, "another versions" should be "other versions."
📝 Proposed fix
Dockerfile and other sources for this container image are available on
https://github.com/sclorg/valkey-container.
-In that repository you also can find another versions of Python environment Dockerfiles.
+In that repository you also can find other versions of Valkey environment Dockerfiles.
Dockerfile for CentOS Stream 10 it's `Dockerfile.c10s` and the Fedora Dockerfile is called Dockerfile.fedora.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| In that repository you also can find another versions of Python environment Dockerfiles. | |
| Dockerfile for CentOS Stream 10 it's `Dockerfile.c10s` and the Fedora Dockerfile is called Dockerfile.fedora. | |
| In that repository you also can find other versions of Valkey environment Dockerfiles. | |
| Dockerfile for CentOS Stream 10 it's `Dockerfile.c10s` and the Fedora Dockerfile is called Dockerfile.fedora. |
🧰 Tools
🪛 LanguageTool
[grammar] ~86-~86: Ensure spelling is correct
Context: ...r. In that repository you also can find another versions of Python environment Dockerfi...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/root/usr/share/container-scripts/valkey/README.md` around lines 86 - 87,
Replace the copy-paste text that incorrectly references "Python environment
Dockerfiles" and the grammar "another versions" in the README: update the
sentence to correctly describe this as the Valkey container image repo and refer
to "other versions" of Dockerfiles, e.g. "In this repository you can also find
other versions of container Dockerfiles; CentOS Stream 10 is Dockerfile.c10s and
Fedora is Dockerfile.fedora" (locate and edit the paragraph containing "Python
environment Dockerfiles" and "another versions").
Pull Request validationFailed🔴 Review - Missing review from a member (1 required) Success🟢 CI - All checks have passed Triggered by Workflow Run |
Pull Request validationFailed🔴 Failed or pending statuses:
Success🟢 Review - Reviewed by a member Triggered by Workflow Run |
phracek
requested changes
Jun 10, 2026
phracek
left a comment
phracek
left a comment
Member
There was a problem hiding this comment.
Thanks. LGTM. Only two issues that should be addressed.
Please remove version '7' also from .github/workflows files.
ndavidova
force-pushed
the
ndavidov/distgen
branch
from
c0c9d88 to
34e8be3
Compare
github-actions
Bot
added
pr/failing-ci
pr/changes-requested
and removed
pr/missing-review
labels
ndavidova
force-pushed
the
ndavidov/distgen
branch
2 times, most recently
from
6ea8796 to
f6809ef
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Dockerfile.fedora`:
- Line 58: The RUN install command currently disables RPM signature verification
via the --nogpgcheck flag; remove --nogpgcheck from the dnf install invocation
in the Dockerfile (the line containing dnf install -y --setopt=tsflags=nodocs
--nogpgcheck $INSTALL_PKGS) so package signatures are validated, and if
necessary ensure required GPG keys are imported earlier in the Dockerfile (e.g.,
add steps to install/import vendor RPM GPG keys before the dnf install) to avoid
verification failures.
In `@src/root/usr/share/container-scripts/valkey/README.md`:
- Line 28: The README uses the outdated literal "rhel8/valkey-{{ spec.version
}}" in multiple example lines; update those occurrences to match the PR's
multispec (RHEL 9/10) by replacing "rhel8/valkey-{{ spec.version }}" with either
the appropriate RHEL target like "rhel9/valkey-{{ spec.version }}" (or
"rhel10/…" where applicable) or, better, generalize the examples to use a
template variable such as "rhel{{ spec.os_major }}/valkey-{{ spec.version }}" so
the sample commands reflect the actual multispec targets; update every instance
of the exact string "rhel8/valkey-{{ spec.version }}" in the README to this
corrected form.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f71f3c0f-49bb-4eb9-b6ba-b7f5ce23c69e
📒 Files selected for processing (46)
.github/workflows/container-tests.yml7/.exclude-c10s7/.exclude-fedora7/.exclude-rhel107/Dockerfile.c10s7/README.md7/test/check_imagestreams.py7/test/examples/valkey-ephemeral-template.json7/test/examples/valkey-persistent-template.json7/test/imagestreams/imagestreams.yaml7/test/imagestreams/valkey-centos.json7/test/run7/test/run-openshift-pytest7/test/run-openshift-remote-cluster7/test/show_all_imagestreams.py7/test/test-lib-openshift.sh7/test/test-lib-remote-openshift.sh7/test/test-lib-valkey.sh7/test/test-lib.sh7/test/test-openshift.yaml7/test/test_valkey_imagestream.py7/test/test_valkey_imagestream_template.py7/test/test_valkey_latest_imagestreams.py7/test/test_valkey_template.py7/test/valkey-persistent-template.json8/Dockerfile.c10s8/Dockerfile.fedora8/Dockerfile.rhel108/Dockerfile.rhel98/testMakefilemanifest.ymlspecs/multispec.ymlsrc/Dockerfilesrc/Dockerfile.fedorasrc/root/usr/bin/container-entrypointsrc/root/usr/bin/run-valkeysrc/root/usr/bin/usagesrc/root/usr/libexec/container-setupsrc/root/usr/share/container-scripts/valkey/README.mdsrc/root/usr/share/container-scripts/valkey/base.conf.templatesrc/root/usr/share/container-scripts/valkey/common.shsrc/root/usr/share/container-scripts/valkey/helpers.shsrc/root/usr/share/container-scripts/valkey/password.conf.templatesrc/root/usr/share/container-scripts/valkey/post-init.shsrc/root/usr/share/container-scripts/valkey/validate-variables.sh
💤 Files with no reviewable changes (21)
- 7/test/test_valkey_latest_imagestreams.py
- 7/test/test_valkey_imagestream_template.py
- 7/README.md
- 7/test/imagestreams/imagestreams.yaml
- 7/test/run-openshift-pytest
- 7/test/test_valkey_imagestream.py
- 7/test/run
- 7/test/test-lib-remote-openshift.sh
- 7/test/run-openshift-remote-cluster
- 7/Dockerfile.c10s
- 7/test/check_imagestreams.py
- 7/test/examples/valkey-persistent-template.json
- 7/test/examples/valkey-ephemeral-template.json
- 7/test/show_all_imagestreams.py
- 7/test/imagestreams/valkey-centos.json
- 7/test/valkey-persistent-template.json
- 7/test/test-lib.sh
- 7/test/test_valkey_template.py
- 7/test/test-lib-openshift.sh
- 7/test/test-lib-valkey.sh
- 7/test/test-openshift.yaml
✅ Files skipped from review due to trivial changes (4)
- 8/Dockerfile.fedora
- 8/test
- 8/Dockerfile.rhel10
- 8/Dockerfile.c10s
🚧 Files skipped from review as they are similar to previous changes (2)
- manifest.yml
- 8/Dockerfile.rhel9
phracek
requested changes
Jun 11, 2026
phracek
left a comment
phracek
left a comment
Member
There was a problem hiding this comment.
The last round review. It looks like examples and imagestreams disappeared.
Any reason why?
Add distgen templates (src/Dockerfile, src/Dockerfile.fedora), multispec configuration, manifest with COPY_RULES for root/ files, and override the generate target in the Makefile. Remove unsupported version 7. Co-Authored-By: Claude Opus 4.6 <[email protected]>
ndavidova
force-pushed
the
ndavidov/distgen
branch
from
f6809ef to
a70052a
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/Dockerfile.fedora (1)
58-58:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove
--nogpgcheckflag to enable package signature verification.Line 58 disables RPM signature verification with
--nogpgcheck, which permits unsigned or tampered packages during the image build. This security concern was raised in a previous review and remains unaddressed.🔒 Proposed fix
- dnf install -y --setopt=tsflags=nodocs --nogpgcheck $INSTALL_PKGS && \ + dnf install -y --setopt=tsflags=nodocs $INSTALL_PKGS && \If GPG key verification fails after this change, ensure the required GPG keys are imported before the package installation step.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Dockerfile.fedora` at line 58, Remove the insecure --nogpgcheck flag from the dnf install command (the line containing "dnf install -y --setopt=tsflags=nodocs --nogpgcheck $INSTALL_PKGS && \\") so RPM signature verification is enabled; if package installs fail after this change, import the required GPG keys (via rpm --import or dnf config-manager --add) before running the dnf install step and ensure the repo GPG keys are configured for the $INSTALL_PKGS sources.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@src/Dockerfile.fedora`:
- Line 58: Remove the insecure --nogpgcheck flag from the dnf install command
(the line containing "dnf install -y --setopt=tsflags=nodocs --nogpgcheck
$INSTALL_PKGS && \\") so RPM signature verification is enabled; if package
installs fail after this change, import the required GPG keys (via rpm --import
or dnf config-manager --add) before running the dnf install step and ensure the
repo GPG keys are configured for the $INSTALL_PKGS sources.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cbb99777-8099-4625-b9c3-60e8312e63d5
📒 Files selected for processing (49)
.github/workflows/container-tests.yml7/.exclude-c10s7/.exclude-fedora7/.exclude-rhel107/Dockerfile.c10s7/README.md7/test/check_imagestreams.py7/test/examples/valkey-ephemeral-template.json7/test/examples/valkey-persistent-template.json7/test/imagestreams/imagestreams.yaml7/test/imagestreams/valkey-centos.json7/test/run7/test/run-openshift-pytest7/test/run-openshift-remote-cluster7/test/show_all_imagestreams.py7/test/test-lib-openshift.sh7/test/test-lib-remote-openshift.sh7/test/test-lib-valkey.sh7/test/test-lib.sh7/test/test-openshift.yaml7/test/test_valkey_imagestream.py7/test/test_valkey_imagestream_template.py7/test/test_valkey_latest_imagestreams.py7/test/test_valkey_template.py7/test/valkey-persistent-template.json8/Dockerfile.c10s8/Dockerfile.fedora8/Dockerfile.rhel108/Dockerfile.rhel98/examples8/imagestreams8/testMakefileREADME.mdmanifest.ymlspecs/multispec.ymlsrc/Dockerfilesrc/Dockerfile.fedorasrc/root/usr/bin/container-entrypointsrc/root/usr/bin/run-valkeysrc/root/usr/bin/usagesrc/root/usr/libexec/container-setupsrc/root/usr/share/container-scripts/valkey/README.mdsrc/root/usr/share/container-scripts/valkey/base.conf.templatesrc/root/usr/share/container-scripts/valkey/common.shsrc/root/usr/share/container-scripts/valkey/helpers.shsrc/root/usr/share/container-scripts/valkey/password.conf.templatesrc/root/usr/share/container-scripts/valkey/post-init.shsrc/root/usr/share/container-scripts/valkey/validate-variables.sh
💤 Files with no reviewable changes (22)
- 7/test/imagestreams/imagestreams.yaml
- 7/test/imagestreams/valkey-centos.json
- 7/README.md
- 7/test/run-openshift-pytest
- 7/test/test_valkey_template.py
- README.md
- 7/test/test-openshift.yaml
- 7/test/check_imagestreams.py
- 7/test/valkey-persistent-template.json
- 7/test/examples/valkey-persistent-template.json
- 7/test/examples/valkey-ephemeral-template.json
- 7/test/test_valkey_latest_imagestreams.py
- 7/test/test_valkey_imagestream.py
- 7/test/run-openshift-remote-cluster
- 7/test/show_all_imagestreams.py
- 7/test/run
- 7/Dockerfile.c10s
- 7/test/test-lib-remote-openshift.sh
- 7/test/test_valkey_imagestream_template.py
- 7/test/test-lib-valkey.sh
- 7/test/test-lib.sh
- 7/test/test-lib-openshift.sh
✅ Files skipped from review due to trivial changes (7)
- 8/examples
- 8/imagestreams
- 8/test
- .github/workflows/container-tests.yml
- 8/Dockerfile.fedora
- 8/Dockerfile.rhel9
- 8/Dockerfile.rhel10
🚧 Files skipped from review as they are similar to previous changes (3)
- 8/Dockerfile.c10s
- specs/multispec.yml
- manifest.yml
phracek
added
the
ready for review
Member
|
Only for hints. Let's test OpenShift. [test-pytest][test-openshift-pytest] |
Member
|
@pkhartsk Please review it as well from your point of view. |
Member
|
OpenShift 4 PyTest issue is filed here: #66 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Chores