packaging: add support for Opensuse Leap 15.6 and SLES 15.7#10978
packaging: add support for Opensuse Leap 15.6 and SLES 15.7#10978illescad wants to merge 14 commits intofluent:masterfrom
Conversation
illescad
requested review from
celalettin1286,
niedbalski and
patrick-stephens
as code owners
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds OpenSUSE Leap 15.6 and SLES 15.7 packaging and install support: documentation and build-config updates, new multi-arch Dockerfiles for both distros, zypper-based test and repo-update flows, update-repos routing to zypper, and install.sh extensions for zypper parameters and install branches. 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 Possibly related PRs
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 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. |
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! |
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
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.
|
@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 |
Thanks, I'll try to look asap but I'm talking at KCD UK next week so have to focus on that. |
|
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 |
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.
There was a problem hiding this comment.
Agreed, we should use args if they are provided.
There was a problem hiding this comment.
@patrick-stephens, thank you for confirming! The FLB_OUT_KAFKA flag should indeed be added to the CMake command in the opensuse Dockerfile to match the pattern used in other distro Dockerfiles. This will ensure the Kafka output plugin configuration is consistent across all packaging targets.
(/￣▽￣)/ ♪
🧠 Learnings used
Learnt from: ThomasDevoogdt
Repo: fluent/fluent-bit PR: 9277
File: .github/workflows/pr-compile-check.yaml:147-151
Timestamp: 2025-08-31T12:46:11.940Z
Learning: In fluent-bit, the correct CMake flag for using system librdkafka is `FLB_PREFER_SYSTEM_LIB_KAFKA=ON`.
Learnt from: ThomasDevoogdt
Repo: fluent/fluent-bit PR: 9277
File: .github/workflows/pr-compile-check.yaml:147-151
Timestamp: 2025-08-31T12:46:11.940Z
Learning: In fluent-bit CMakeLists.txt, the system library preference flags are defined as FLB_PREFER_SYSTEM_LIB_ZSTD and FLB_PREFER_SYSTEM_LIB_KAFKA with the FLB_ prefix.
Learnt from: cosmo0920
Repo: fluent/fluent-bit PR: 10832
File: src/wasm/CMakeLists.txt:112-131
Timestamp: 2025-09-04T07:28:37.083Z
Learning: In fluent-bit CMake files, the user cosmo0920 prefers treating Git as a command rather than a package, emphasizing that Git is not a pkg-config retrievable package but just a command.
|
@patrick-stephens things are looking good! |
|
@patrick-stephens i believe all questions were addressed/resolved. Can we get this merged in now? |
|
Sorry, my notifications have been overwhelmed since the holiday season and my jury service after it so will take a look shortly. It looks like we still have a comment and some failing CI though. |
install.sh
Outdated
| cat << EOF > /etc/zypp/repos.d/fluent-bit.repo | ||
| [fluent-bit] | ||
| name = Fluent Bit | ||
| baseurl = $RELEASE_URL/opensuse/leap/\$releaserver |
There was a problem hiding this comment.
Can you confirm this works to only substitute the correct thing? I notice in the other sections we do a sed afterwards instead so we should be consistent in both.
There was a problem hiding this comment.
The reason I ask is I think I had issues before with it being too aggressive on substituting variables in the HEREDOC even with escaping, you can also quote the EOF to prevent it but in this case you need to substitute the RELEASE_URL. I think what you have is correct but I'd just like a quick test to confirm in a real world install.
|
@illescad can I give you a quick poke on this so we can merge? |
hey yea i'd like to dedicate some time to get this wrapped up as well. right now it looks like a quick code rabbit fix and test out the install.sh? Anything else im missing?? |
Nope, that's all - if you can rebase as well to make sure we've got all the latest changes in particularly around the CI due to the recent changes to mitigate Github security issues. |
illescad
force-pushed
the
illescad-sles-packaging
branch
from
fc3e918 to
88968e3
Compare
…is not working. Currently testing ./build.sh -d opensuse/leap:15.6 Signed-off-by: Dan Illescas <daniel.illescas@hpe.com>
Signed-off-by: Dan Illescas <daniel.illescas@hpe.com>
…upport suse Signed-off-by: Dan Illescas <daniel.illescas@hpe.com>
Signed-off-by: Dan Illescas <daniel.illescas@hpe.com>
…n test-release-packages.sh Signed-off-by: Dan Illescas <daniel.illescas@hpe.com>
Signed-off-by: Dan Illescas <daniel.illescas@hpe.com>
…ome coderabbit concerns Signed-off-by: Dan Illescas <daniel.illescas@hpe.com>
Signed-off-by: Dan Illescas <daniel.illescas@hpe.com>
Signed-off-by: Dan Illescas <daniel.illescas@hpe.com>
…clean up Signed-off-by: Dan Illescas <daniel.illescas@hpe.com>
Signed-off-by: Dan Illescas <daniel.illescas@hpe.com>
Signed-off-by: Dan Illescas <daniel.illescas@hpe.com>
illescad
force-pushed
the
illescad-sles-packaging
branch
from
88968e3 to
46db7ff
Compare
|
@patrick-stephens I've updated install.sh for opensuse and sles to match other distros replace with sed. Added in the kafka_out flag to cmake for both distros rebased to upstream/master |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
install.sh (1)
151-189: Consider extracting the duplicated zypper install flow into a helper.The
opensuse-leapandslesblocks are nearly identical; a small helper would reduce drift risk (like the macro typo) and simplify future updates.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@install.sh` around lines 151 - 189, Extract the duplicated zypper flow in the opensuse-leap and sles case blocks into a helper function (e.g., setup_zypper_repo_and_install) that accepts the distro path segment (opensuse/leap vs sles) and the VERSION_SUBSTR replacement token, then move the shared sequence—rpm --import $RELEASE_KEY, heredoc to create /etc/zypp/repos.d/fluent-bit.repo using $RELEASE_URL and $RELEASE_KEY, the sed substitution, zypper refresh and the install command—into that helper; call it from both case labels passing the appropriate URL segment and replacement token (ensure you pass the correct replacement string used in sed: "$releasever" for sles and the intended token for opensuse-leap), and reuse existing variables $INSTALL_CMD_PREFIX, $ZYPPER_PARAMETERS, $INSTALL_PACKAGE_NAME and $ZYPPER_VERSION so the final behavior is identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@install.sh`:
- Line 165: The sed substitution on the line using sed -i
's|VERSION_SUBSTR|\$releaserver|g' should use the correct openSUSE zypper macro
name; change the replacement token from \$releaserver to \$releasever so the
command becomes sed -i 's|VERSION_SUBSTR|\$releasever|g' (locate the sed call
that replaces VERSION_SUBSTR in install.sh).
---
Nitpick comments:
In `@install.sh`:
- Around line 151-189: Extract the duplicated zypper flow in the opensuse-leap
and sles case blocks into a helper function (e.g.,
setup_zypper_repo_and_install) that accepts the distro path segment
(opensuse/leap vs sles) and the VERSION_SUBSTR replacement token, then move the
shared sequence—rpm --import $RELEASE_KEY, heredoc to create
/etc/zypp/repos.d/fluent-bit.repo using $RELEASE_URL and $RELEASE_KEY, the sed
substitution, zypper refresh and the install command—into that helper; call it
from both case labels passing the appropriate URL segment and replacement token
(ensure you pass the correct replacement string used in sed: "$releasever" for
sles and the intended token for opensuse-leap), and reuse existing variables
$INSTALL_CMD_PREFIX, $ZYPPER_PARAMETERS, $INSTALL_PACKAGE_NAME and
$ZYPPER_VERSION so the final behavior is identical.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
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