packaging: add support for Opensuse Leap 15.6 and SLES 15.7 #10978
Conversation
illescad
requested review from
celalettin1286,
niedbalski and
patrick-stephens
as code owners
WalkthroughAdds OpenSUSE Leap 15.6 and SLES 15.7 packaging support: updates docs and build-config, introduces multi-arch Dockerfiles for both distros, adds zypper-based test and repo-update flows, routes repo updates to zypper for SUSE paths, and extends the installer with zypper parameters and a zypper branch. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant Installer as install.sh
participant Repo as Fluent-Bit Repo
participant Zypper
User->>Installer: run installer
Installer->>Installer: detect OS (opensuse/leap or sles)
alt SUSE family
Installer->>Repo: import GPG key
Installer->>Installer: write `/etc/zypp/repos.d/fluent-bit.repo`
Installer->>Zypper: refresh metadata
Installer->>Zypper: install fluent-bit (ZYPPER_VERSION, ZYPPER_PARAMETERS)
Zypper-->>User: installation result
else other OS families
Installer->>Installer: run apt/yum flows
end
sequenceDiagram
autonumber
participant CI
participant Updater as update-repos.sh
participant YumUpd as update-yum-repo.sh
participant ZypperUpd as update-zypper-repo.sh
CI->>Updater: run repo update
Updater->>Updater: iterate RPM_REPO_PATHS
alt path matches opensuse/* or sles/*
Updater->>ZypperUpd: run zypper repo update (sign, createrepo, optional .repo)
else
Updater->>YumUpd: run yum repo update
end
sequenceDiagram
autonumber
participant Tester
participant TestScript as test-release-packages.sh
participant Docker
participant Zypper
Tester->>TestScript: run tests
loop for each ZYPPER_TARGET
TestScript->>Docker: run image
Docker->>Zypper: refresh, install gpg/curl
Docker->>Zypper: run install script (install.sh)
Docker-->>TestScript: return `fluent-bit --version`
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Areas to focus review on:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packaging/README.md(1 hunks)packaging/build-config.json(1 hunks)packaging/distros/opensuse/15.6.arm64v8/Dockerfile(1 hunks)packaging/distros/opensuse/15.6/Dockerfile(1 hunks)packaging/distros/opensuse/Dockerfile(1 hunks)packaging/distros/sles/Dockerfile(1 hunks)packaging/test-release-packages.sh(1 hunks)packaging/update-repos.sh(1 hunks)
🔇 Additional comments (3)
packaging/distros/opensuse/15.6.arm64v8/Dockerfile (1)
12-24: Confirm availability ofcmake3-full.On Leap 15.x the package is usually
cmake/cmake-full;cmake3-fullmay not exist and would break the build. Please confirm the package name and adjust if necessary (likelycmake-full).packaging/distros/opensuse/15.6/Dockerfile (1)
7-19: Verify thecmake3-fulldependency.Same concern as the arm64 Dockerfile: ensure
cmake3-fullexists on Leap 15.6; otherwise switch tocmake-fullto avoid install failures.packaging/distros/opensuse/Dockerfile (1)
16-28: Please double-check thecmake3-fullinstall.For the generic OpenSUSE builder we also request confirmation that
cmake3-fullis present on the chosen Leap image; if not, use the availablecmake-fullpackage instead.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packaging/README.md (1)
40-43: Docs update looks good; minor style nitEntries are correct. Consider dropping bold on “SLES” to match the table’s neutral style used for other distros.
packaging/test-release-packages.sh (1)
123-141: Harden zypper invocations to avoid key promptsAdd --gpg-auto-import-keys to refresh/install to eliminate potential non-interactive key prompts on fresh images.
- sh -c "zypper --non-interactive refresh && zypper --non-interactive install gpg curl; $INSTALL_CMD && /opt/fluent-bit/bin/fluent-bit --version" | tee "$LOG_FILE" + sh -c "zypper --non-interactive --gpg-auto-import-keys refresh && zypper --non-interactive --gpg-auto-import-keys install gpg curl; $INSTALL_CMD && /opt/fluent-bit/bin/fluent-bit --version" | tee "$LOG_FILE"install.sh (2)
158-172: Avoid non-interactive key prompts on repo refresh/installAdd --gpg-auto-import-keys to the zypper commands to ensure fully non-interactive runs across images.
- zypper --non-interactive refresh - $INSTALL_CMD_PREFIX zypper --non-interactive $ZYPPER_PARAMETERS install $INSTALL_PACKAGE_NAME$ZYPPER_VERSION + zypper --non-interactive --gpg-auto-import-keys refresh + $INSTALL_CMD_PREFIX zypper --non-interactive --gpg-auto-import-keys $ZYPPER_PARAMETERS install $INSTALL_PACKAGE_NAME$ZYPPER_VERSION
161-169: Optional repo metadata fieldsConsider adding type=rpm-md and autorefresh=1 for clarity and default behavior parity with common SUSE repo snippets. Not required.
[fluent-bit] name = Fluent Bit baseurl = $RELEASE_URL/$SUSE_OS/$SUSE_VER gpgcheck=1 repo_gpgcheck=1 gpgkey=$RELEASE_KEY enabled=1 +type=rpm-md +autorefresh=1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
install.sh(3 hunks)packaging/README.md(1 hunks)packaging/test-release-packages.sh(2 hunks)
🔇 Additional comments (4)
packaging/test-release-packages.sh (1)
63-66: Good targetsCorrect images for openSUSE Leap and SLES (BCI base). Nice fix separating Zypper-based distros.
install.sh (3)
17-17: Env parameter passthrough looks goodZYPPER_PARAMETERS addition mirrors APT/YUM patterns. No issues.
52-57: Version pinning parityZYPPER_VERSION addition aligns with YUM_VERSION format. Good for consistent pinning.
151-172: Consistency check passed: packaging/update-repos.sh and packaging/build-config.json already include opensuse/15.6 and sles/15.7.
|
@illescad can you raise and link a docs PR as well? Also just tweak the commits and PR title to include |
illescad
force-pushed
the
illescad-sles-packaging
branch
from
75c6d92 to
f52823d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
install.sh(3 hunks)packaging/README.md(1 hunks)packaging/build-config.json(1 hunks)packaging/distros/opensuse/15.6.arm64v8/Dockerfile(1 hunks)packaging/distros/opensuse/15.6/Dockerfile(1 hunks)packaging/distros/opensuse/Dockerfile(1 hunks)packaging/distros/sles/Dockerfile(1 hunks)packaging/test-release-packages.sh(2 hunks)packaging/update-repos.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- packaging/README.md
- packaging/distros/sles/Dockerfile
- install.sh
- packaging/distros/opensuse/15.6/Dockerfile
- packaging/build-config.json
- packaging/distros/opensuse/Dockerfile
- packaging/update-repos.sh
| RUN zypper up -y && \ | ||
| zypper install -y --no-recommends \ | ||
| rpm-build \ | ||
| curl ca-certificates wget unzip flex bison \ | ||
| gcc gcc-c++ \ | ||
| cmake3-full \ | ||
| make \ | ||
| bash \ | ||
| systemd-devel \ | ||
| postgresql postgresql-devel postgresql-server \ | ||
| cyrus-sasl cyrus-sasl-devel \ | ||
| libopenssl3 libopenssl-3-devel \ | ||
| libyaml-devel && \ | ||
| zypper clean -a && rm -rf /var/cache/zypp/* |
There was a problem hiding this comment.
cmake3-full doesn’t exist in Leap 15.x repos
zypper install … cmake3-full will fail because the package name in Leap/SLES is cmake (with the optional cmake-full pattern), not cmake3-full. The Docker build will therefore break before we even reach CMake configure. Please swap in the correct package name (typically cmake or cmake-full) so the image can install successfully.
🤖 Prompt for AI Agents
In packaging/distros/opensuse/15.6.arm64v8/Dockerfile around lines 11-24, the
Dockerfile attempts to install a non-existent package named cmake3-full on Leap
15.x; replace cmake3-full with the correct package name (typically cmake or the
cmake-full pattern) in the zypper install line so the install succeeds, then
rebuild the image to verify CMake is available.
packaging/test-release-packages.sh
Outdated
| sh -c "zypper --non-interactive --gpg-auto-import-keys refresh \ | ||
| zypper --non-interactive --gpg-auto-import-keys install gpg curl; \ | ||
| $INSTALL_CMD /opt/fluent-bit/bin/fluent-bit --version" | tee "$LOG_FILE" |
There was a problem hiding this comment.
Fix command chaining in the zypper loop
The escaped newline glues both zypper invocations into a single command (zypper … refresh zypper … install …), which fails, and $INSTALL_CMD /opt/… treats the version check as arguments to the installer, so /opt/fluent-bit/bin/fluent-bit --version never runs. Use explicit && separators just like the YUM/APT blocks.
- sh -c "zypper --non-interactive --gpg-auto-import-keys refresh \
- zypper --non-interactive --gpg-auto-import-keys install gpg curl; \
- $INSTALL_CMD /opt/fluent-bit/bin/fluent-bit --version" | tee "$LOG_FILE"
+ sh -c "zypper --non-interactive --gpg-auto-import-keys refresh && \
+ zypper --non-interactive --gpg-auto-import-keys install gpg curl && \
+ $INSTALL_CMD && /opt/fluent-bit/bin/fluent-bit --version" | tee "$LOG_FILE"📝 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.
| sh -c "zypper --non-interactive --gpg-auto-import-keys refresh \ | |
| zypper --non-interactive --gpg-auto-import-keys install gpg curl; \ | |
| $INSTALL_CMD /opt/fluent-bit/bin/fluent-bit --version" | tee "$LOG_FILE" | |
| sh -c "zypper --non-interactive --gpg-auto-import-keys refresh && \ | |
| zypper --non-interactive --gpg-auto-import-keys install gpg curl && \ | |
| $INSTALL_CMD && /opt/fluent-bit/bin/fluent-bit --version" | tee "$LOG_FILE" |
🤖 Prompt for AI Agents
In packaging/test-release-packages.sh around lines 139 to 141, the zypper
commands are joined by an escaped newline which concatenates them into a single
invalid command and causes the version check to be passed as arguments to the
installer; change the chaining to use explicit && between commands (e.g. zypper
... refresh && zypper ... install gpg curl && $INSTALL_CMD
/opt/fluent-bit/bin/fluent-bit --version) so each zypper invocation runs
sequentially and the --version check executes as a separate command.
f52823d to
f15e9e4
Compare
patrick-stephens
changed the title
|
@illescad if you can sort the DCO and docs PR that would be great so we can merge? Plus just check the CodeRabbit comments and resolve/fix as needed. |
…is not working. Currently testing ./build.sh -d opensuse/leap:15.6 Signed-off-by: Dan Illescas <[email protected]>
Signed-off-by: Dan Illescas <[email protected]>
…upport suse Signed-off-by: Dan Illescas <[email protected]>
Signed-off-by: Dan Illescas <[email protected]>
…n test-release-packages.sh Signed-off-by: Dan Illescas <[email protected]>
illescad
force-pushed
the
illescad-sles-packaging
branch
from
c6c3a76 to
3389284
Compare
|
@patrick-stephens working through the the docs PR today and any coderabbit fixes |
Thanks mate, let me know when you're done and I'll re-approve for merge. Looks good to me and I'll probably take it downstream to our repo straightaway anyway! |
Signed-off-by: Dan Illescas <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
packaging/update-zypper-repo.sh (7)
2-2: Harden shell optionsAdd pipefail to catch errors in pipelines; optionally gate xtrace with a DEBUG flag.
-set -eux +set -euo pipefail +${DEBUG:+set -x}
36-41: realpath flags portability; add fallback
realpath -smmay not be available on all images. Provide a fallback toreadlink -for drop-s.-REPO_DIR=$(realpath -sm "$BASE_PATH/$RPM_REPO") +if command -v realpath >/dev/null 2>&1; then + REPO_DIR=$(realpath -m "$BASE_PATH/$RPM_REPO") +elif command -v readlink >/dev/null 2>&1; then + REPO_DIR=$(readlink -f "$BASE_PATH/$RPM_REPO") +else + REPO_DIR=$(cd "$BASE_PATH/$RPM_REPO" && pwd -P) +fiPlease confirm
realpath -sexists on your target SUSE/Leap images.
42-46: RPM signing: robustness and batching
- Use null-delimited paths and batch to reduce process spawn and handle odd filenames.
- Ensure we only target regular files.
- find "$REPO_DIR" -name "*-bit-*.rpm" -exec rpm --define "_gpg_name $GPG_KEY" --addsign {} \; + find "$REPO_DIR" -type f -name "*-bit-*.rpm" -print0 | \ + xargs -0 -r rpm --define "_gpg_name $GPG_KEY" --addsign
50-66: Align metadata signing with zypper checksYou sign
repomd.xml, but the generated .repo lacksrepo_gpgcheck=1. Either add it (recommended) or skip metadata signing.baseurl=https://$AWS_S3_BUCKET.s3.amazonaws.com/$RPM_REPO/ enabled=1 gpgkey=https://$AWS_S3_BUCKET.s3.amazonaws.com/fluentbit.key gpgcheck=1 +repo_gpgcheck=1 autorefresh=1If you prefer not to enforce repo metadata verification, remove the repomd signatures to avoid confusion.
Also applies to: 68-76
60-63: Allow overriding base URL (CDN/region)Hardcoding the S3 endpoint can be limiting. Support
REPO_BASEURLoverride while keeping current default.-baseurl=https://$AWS_S3_BUCKET.s3.amazonaws.com/$RPM_REPO/ +baseurl=${REPO_BASEURL:-https://$AWS_S3_BUCKET.s3.amazonaws.com/$RPM_REPO/}
29-31: Package name hintOn SUSE, the package providing
createrepois typicallycreaterepo_c. Adjust helper text.-echo "ERROR: 'createrepo' command not found. Please install it, e.g., 'zypper install createrepo'." +echo "ERROR: 'createrepo' command not found. Install it, e.g., 'zypper install createrepo_c'."
1-3: Set a sane umask for repo filesEnsure world-readable artifacts.
#!/bin/bash -set -euo pipefail +umask 022 +set -euo pipefail
There was a problem hiding this comment.
I don't think you need this as it should be part of the opensuse/Dockerfile?
There was a problem hiding this comment.
I had an issue with docker finding the correct base image when trying to set the specific leap versions. So if using the opensuse/Dockerfile and setting the target to something like
./packaging/build.sh -d opensuse/leap:15.6
Then the build.sh will add a -base to the image name, opensuse/leap:15.6-base, which does not exist for opensuse. The standard naming convention is opensuse/leap:15.6 which is already the minimal image.
I wasnt sure how to correctly get around this, I can also add a check in build.sh if DISTRO is opensuse to not append the -base. Or maybe there is another way I am not understanding
There was a problem hiding this comment.
I don't think you need this as it should be part of the opensuse/Dockerfile?
There was a problem hiding this comment.
I think i discovered the problem i had on why i thought i needed multiple Dockerfiles. The build.sh needs to be updated to replace : with - if i want to call ./build.sh -d opensuse/leap:15.6. I'll push an update to remove the extra Dockerfiles.
Is there a preference on the name we will pass the target distro to build.sh?
./build.sh -d opensuse/leap:15.6
or
./build.sh -d opensuse/leap-15.6
or
./build.sh -d opensuse/15.6
opensuse/leap:15.6 makes sense to me and I can make a change to build.sh to replace : with - to make sure its a valid stage name.
There was a problem hiding this comment.
I think we've typically done the os/version approach in the past - you can see with centos for example the actual base images are stream now but we kept the old name for consistency.
I would probably just go with opensuse/15.6 for now to keep it simple - otherwise we start running into making typos or trying to use non-leap versions, etc.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packaging/README.md (1)
40-43: Docs addition looks consistent; consider noting Tumbleweed unsupportedTargets and “Target Option” values align with build.sh and repo paths. Optional: add a short note that openSUSE Tumbleweed isn’t supported via install.sh to avoid confusion.
packaging/update-zypper-repo.sh (2)
28-39: Honor CREATE_REPO_CMD in availability check and messageYou define CREATE_REPO_CMD but check for a hardcoded createrepo. Use the variable for consistency and overrides.
-# Check for createrepo -if ! command -v createrepo &> /dev/null; then - echo "ERROR: 'createrepo' command not found. Please install it, e.g., 'zypper install createrepo'." +# Check for createrepo-compatible command +if ! command -v "$CREATE_REPO_CMD" &> /dev/null; then + echo "ERROR: '$CREATE_REPO_CMD' not found. Install it, e.g., 'zypper install createrepo'." exit 1 fi
49-53: Robust signing: handle spaces and show failuresMinor hardening: use -print0 with xargs and fail the script if any signing fails.
-if [[ "$DISABLE_SIGNING" != "true" ]]; then - # Sign all RPMs created for this target, cover both fluent-bit and legacy packages - find "$REPO_DIR" -name "*-bit-*.rpm" -exec rpm --define "_gpg_name $GPG_KEY" --addsign {} \; -fi +if [[ "$DISABLE_SIGNING" != "true" ]]; then + # Sign all RPMs (fluent-bit and legacy td-agent-bit) + find "$REPO_DIR" -name "*-bit-*.rpm" -print0 | xargs -0 -r -n1 \ + rpm --define "_gpg_name $GPG_KEY" --addsign +fipackaging/update-repos.sh (1)
61-68: Pass BASE_PATH explicitly to child scripts to reduce env couplingupdate-zypper-repo.sh expects BASE_PATH via env or $1. Pass it to be explicit and resilient.
- "opensuse/"* | "sles/"*) - /bin/bash -eux "$SCRIPT_DIR/update-zypper-repo.sh" + "opensuse/"* | "sles/"*) + /bin/bash -eux "$SCRIPT_DIR/update-zypper-repo.sh" "$BASE_PATH" ;; *) - /bin/bash -eux "$SCRIPT_DIR/update-yum-repo.sh" + /bin/bash -eux "$SCRIPT_DIR/update-yum-repo.sh" "$BASE_PATH" ;;packaging/distros/sles/Dockerfile (1)
63-71: CMake flags: remove unused FLB_OUT_KAFKA arg or wire it throughYou define FLB_OUT_KAFKA but pass FLB_KAFKA to CMake. Either drop FLB_OUT_KAFKA or add the corresponding -DFLB_OUT_KAFKA flag if intended.
-ARG FLB_OUT_KAFKA=On ... - -DFLB_KAFKA="$FLB_KAFKA" \ + -DFLB_KAFKA="$FLB_KAFKA" \ + # If separate output toggle exists, include: + # -DFLB_OUT_KAFKA="$FLB_OUT_KAFKA" \
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
install.sh(3 hunks)packaging/README.md(1 hunks)packaging/build-config.json(1 hunks)packaging/build.sh(1 hunks)packaging/distros/opensuse/Dockerfile(1 hunks)packaging/distros/sles/Dockerfile(1 hunks)packaging/update-repos.sh(2 hunks)packaging/update-zypper-repo.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packaging/distros/opensuse/Dockerfile
- packaging/build-config.json
🔇 Additional comments (5)
install.sh (2)
151-169: openSUSE Leap zypper branch LGTMRepo stanza, key import, refresh, and install flow look correct for Leap using VERSION_ID.
171-188: Verify zypper’s $releasever expansion on SLES 15.7
Test on a SLES 15.7 instance to confirm$releaseveris correctly expanded in the repo URL; if it isn’t, replace with$VERSION_ID(or a sanitized SLES version):- baseurl = $RELEASE_URL/sles/\$releasever + baseurl = $RELEASE_URL/sles/$VERSION_IDpackaging/update-repos.sh (1)
46-48: Repo list update LGTMNew SUSE paths present: opensuse/leap/15.6 and sles/15.7.
packaging/build.sh (1)
63-71: Stage name derivation matches versioned base stagesConstructing BASE_BUILDER as "-<cleaned_version>-base" aligns with SLES stage names (e.g., sles-15.7-base) and likely the opensuse/leap:15.6 Dockerfile. Good.
If opensuse’s Dockerfile uses unversioned base stage names, update it to include the version or adjust this derivation accordingly.
packaging/distros/sles/Dockerfile (1)
9-20: Base images and tooling look correct for SLES 15.7Multi-arch bases, qemu setup, zypper install set, and cleanup steps are appropriate.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packaging/README.md(1 hunks)packaging/build-config.json(1 hunks)packaging/distros/opensuse/Dockerfile(1 hunks)packaging/distros/sles/Dockerfile(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packaging/distros/sles/Dockerfile
…ome coderabbit concerns Signed-off-by: Dan Illescas <[email protected]>
illescad
force-pushed
the
illescad-sles-packaging
branch
from
7590d18 to
04fd8bc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packaging/update-repos.sh (1)
61-68: Avoid quoting case patterns; use unquoted globsQuoting patterns in case may be misleading and can inhibit pattern semantics in some shells. Prefer unquoted patterns.
- case "$RPM_REPO" in - "opensuse/"* | "sles/"*) + case "$RPM_REPO" in + opensuse/* | sles/*) /bin/bash -eux "$SCRIPT_DIR/update-zypper-repo.sh" ;; *) /bin/bash -eux "$SCRIPT_DIR/update-yum-repo.sh" ;; esacinstall.sh (1)
151-170: LGTM: openSUSE Leap repo and install flow
- Uses VERSION_ID for Leap (15.6) path.
- Properly imports key, sets rpm-md repo, refreshes, installs with non-interactive.
One nit: consider adding a brief comment that Leap requires VERSION_ID whereas SLES uses $releasever to avoid future drift.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
install.sh(3 hunks)packaging/distros/opensuse/Dockerfile(1 hunks)packaging/distros/sles/Dockerfile(1 hunks)packaging/update-repos.sh(2 hunks)packaging/update-zypper-repo.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packaging/distros/sles/Dockerfile
- packaging/distros/opensuse/Dockerfile
- packaging/update-zypper-repo.sh
🔇 Additional comments (3)
packaging/update-repos.sh (1)
46-48: LGTM: SUSE/SLES repo paths addedNew RPM targets look correct and align with the installer paths.
Please confirm packages.fluentbit.io has these directories populated before enabling AWS_SYNC.
install.sh (2)
17-17: LGTM: New ZYPPER_PARAMETERS knobKeeps CLI flexibility consistent with APT/YUM.
171-188: Verify SLES $releasever expands to 15.7 Ensure on all supported SLES images that$releaseverresolves to 15.7 (SLE 15 SP7); if it yields15or15-SP7, add a mapping or fallback to target SP7.
Signed-off-by: Dan Illescas <[email protected]>
|
@illescad let me know when you need another review. |
|
@patrick-stephens it's ready for another review. I'll be trying to get some progress on the docs PR |
Signed-off-by: Dan Illescas <[email protected]>
Thanks, I'll try to look asap but I'm talking at KCD UK next week so have to focus on that. |
|
@patrick-stephens can you give this a look (review), as docs PR is ready too: fluent/fluent-bit-docs#2103 |
|
hey @patrick-stephens wanted to follow up on another review for this, it's ready to go |
|
Yeah sorry, school holidays and KCD UK distracted me so apologies for that. Will have a look this week. |
patrick-stephens
left a comment
patrick-stephens
left a comment
There was a problem hiding this comment.
Just some minor niggles to sort.
|
moved to next milestone (not ready) |
…clean up Signed-off-by: Dan Illescas <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packaging/distros/opensuse/Dockerfile (1)
11-29: Note: Substantial code duplication across three base stages.The three opensuse base stages (opensuse-leap-base, opensuse-15.6-base, opensuse-15.6.arm64v8-base) each contain identical zypper package lists and cleanup commands. While Docker limitations make it difficult to factor this out without external tooling, consider documenting in the repo's README why this duplication exists and whether a shared base pattern could be introduced in a future refactor.
For this PR, the identical package lists across all three stages are acceptable and ensure consistency.
Also applies to: 31-48, 50-70
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packaging/distros/opensuse/Dockerfile(1 hunks)packaging/distros/sles/Dockerfile(1 hunks)
🔇 Additional comments (2)
packaging/distros/sles/Dockerfile (1)
1-88: Package selection and multi-arch setup look solid.The SLES Dockerfile correctly uses SLES-native packages (cmake-full, libopenssl-devel) and follows the same multi-stage, multi-arch pattern as the OpenSUSE variant. The qemu-aarch64-static setup for ARM64 is correct, and the builder stage CMake configuration is appropriate.
packaging/distros/opensuse/Dockerfile (1)
11-48: Package names and multi-arch setup are correct.The OpenSUSE Dockerfile correctly uses distro-native packages (cmake-full, libopenssl-devel) across all base stages, and the multi-arch setup with qemu-aarch64-static is properly configured. Prior issues with incorrect package names (cmake3-full, libopenssl-3-devel) appear to have been resolved.
Signed-off-by: Dan Illescas <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packaging/distros/opensuse/Dockerfile (1)
15-29: Recommended refactor: reduce duplication in base stage package lists.The three base stages (lines 15–29, 34–48, 56–70) have nearly identical zypper install commands differing only in the FROM image and platform specifics. You could reduce duplication by defining a shared base template or using a heredoc, though Docker's multi-stage structure makes this less critical. If this pattern extends to other Dockerfiles in the PR, consider a shared base-layer approach for maintainability.
Also applies to: 34-48, 56-70
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packaging/distros/opensuse/Dockerfile(1 hunks)
🔇 Additional comments (1)
packaging/distros/opensuse/Dockerfile (1)
78-121: LGTM: builder stage and CMake configuration look solid.The common builder stage (line 78) correctly uses the ARG BASE_BUILDER parameter to select the appropriate base image, and the CMake configuration (lines 106–121) is comprehensive with all relevant Fluent Bit build flags (SIMD, TRACE, HTTP_SERVER, KAFKA, PostgreSQL, Jemalloc, etc.). The build and packaging workflow (line 124) is correct. No concerns here.
|
@illescad any chance you can finish this off? |
|
I have time this week to wrap this up. I thought i had any issues addressed already, is there something specific in the PR that i am missing? |
|
You might have done it all, sorry it's hard with all my notifications! If you can just double check and ping me then I can merge if so. |
|
@patrick-stephens ok double checked and any of your previous notes have been fixed/addressed |
|
Just the OpenSSL 3 headers question |
Signed-off-by: Dan Illescas <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packaging/distros/opensuse/Dockerfile (2)
32-32: Remove trailing whitespace.Line 32 has trailing whitespace after the stage name.
-FROM opensuse/leap:15.6 AS opensuse-15.6-base +FROM opensuse/leap:15.6 AS opensuse-15.6-base
124-124: Consider dynamic parallelism for make.The hardcoded
-j 4is a safe default but may underutilize systems with more cores. Consider using$(nproc)for dynamic parallelism, or document why 4 was chosen.-CMD [ "/bin/bash", "-c", "make --no-print-directory -j 4 && cpack -G RPM && cp *.rpm /output/" ] +CMD [ "/bin/bash", "-c", "make --no-print-directory -j $(nproc) && cpack -G RPM && cp *.rpm /output/" ]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packaging/distros/opensuse/Dockerfile(1 hunks)packaging/distros/sles/Dockerfile(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packaging/distros/sles/Dockerfile
🔇 Additional comments (3)
packaging/distros/opensuse/Dockerfile (3)
1-9: LGTM!The header documentation clearly explains the BASE_BUILDER mechanism and the multiarch setup using qemu-user-static is a standard pattern for cross-architecture builds.
11-29: Floating tag retained for generic Leap builds.The
opensuse/leapfloating tag (line 13) appears to be intentionally retained to provide a "latest Leap" build target, whileopensuse-15.6-baseprovides a pinned alternative. This is a reasonable design given theBASE_BUILDERARG mechanism allows selecting the appropriate base. Consider adding a brief comment explaining this distinction for future maintainers.
50-74: LGTM!The arm64 base stage correctly uses
--platform=linux/arm64syntax, includes the qemu static binary for cross-compilation, and appropriately configures jemalloc options with larger page size (--with-lg-page=16) suitable for arm64 architectures.
| ARG FLB_TRACE=On | ||
| ARG FLB_SQLDB=On | ||
| ARG FLB_HTTP_SERVER=On | ||
| ARG FLB_OUT_KAFKA=On |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify if FLB_OUT_KAFKA is a valid CMake option and how it's used
echo "=== Checking for FLB_OUT_KAFKA in Dockerfiles ==="
rg -n 'FLB_OUT_KAFKA' --type-add 'docker:Dockerfile*' --type docker
echo "=== Checking CMakeLists for FLB_OUT_KAFKA ==="
rg -n 'FLB_OUT_KAFKA' -g 'CMakeLists.txt'
echo "=== Checking for FLB_OUT_KAFKA in all files ==="
rg -n 'FLB_OUT_KAFKA' --max-count 20
echo "=== Checking opensuse Dockerfile context around line 98 ==="
sed -n '90,125p' packaging/distros/opensuse/DockerfileRepository: fluent/fluent-bit
Length of output: 4622
Add missing FLB_OUT_KAFKA flag to CMake command.
FLB_OUT_KAFKA is defined as an ARG on line 98 but is not passed to CMake (lines 106-121). Other Dockerfiles (debian, ubuntu, raspbian, etc.) include -DFLB_OUT_KAFKA="$FLB_OUT_KAFKA" in their CMake invocations. Add this flag to the opensuse Dockerfile's cmake command to ensure consistent configuration.
🤖 Prompt for AI Agents
In packaging/distros/opensuse/Dockerfile around lines 98 and 106-121, the ARG
FLB_OUT_KAFKA is declared on line 98 but not forwarded to the CMake invocation;
update the CMake command(s) in the block at lines ~106-121 to include
-DFLB_OUT_KAFKA="$FLB_OUT_KAFKA" alongside the other -D flags so the Kafka
output option honors the build ARG the same way other distro Dockerfiles do.
|
@patrick-stephens things are looking good! |
4 participants
Providing support for SLES and Opensuse
Resolves #10875 by adding SLES v15.7 and Opensuse Leap 15.6 packaging. Opensuse repo images are minimal so adding -base to the target will case errors. To get around this multiple Dockerfiles were created for distro/opensuse/15.6*
Testing
Before we can approve your change; please submit the following in a comment:
[N/A] Example configuration file for the change
[N/A] Debug log output from testing the change
[N/A] Attached Valgrind output that shows no leaks or memory corruption was found
If this is a change to packaging of containers or native binaries then please confirm it works for all targets.
ok-package-testlabel to test for all targets (requires maintainer to do).Documentation
Backporting
Fluent Bit is licensed under Apache 2.0, by submitting this pull request I understand that this code will be released under the terms of that license.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.