Open
Conversation
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
edsiper
requested review from
celalettin1286,
niedbalski and
patrick-stephens
as code owners
📝 WalkthroughWalkthroughReplaces the inline Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant GH as GitHub Actions
participant Runner as Actions Runner
participant Script as sync-apt-repo-to-s3.sh
participant FS as Local Repo (packaging/releases/$CODENAME)
participant S3 as AWS S3
GH->>Runner: trigger staging-release job
Runner->>Script: run ./packaging/sync-apt-repo-to-s3.sh <source> <s3-dest>
Script->>FS: validate source dir, normalize path
Script->>S3: aws s3 sync <source> <s3-dest> (exclude Release files)
Script->>FS: locate dists/*/{InRelease,Release,Release.gpg}
loop for each metadata file
Script->>S3: aws s3 cp <file> <s3-dest>/<relative-path>
end
Script-->>Runner: exit (success/failure)
Runner-->>GH: job result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fc7069b804
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/staging-release.yaml:
- Around line 277-279: The workflow step invoking
./packaging/sync-apt-repo-to-s3.sh in the "Sync to release bucket on S3" job
fails because that helper script is missing; fix by either adding the script at
packaging/sync-apt-repo-to-s3.sh implementing the aws s3 sync call with the same
flags used elsewhere (--delete --follow-symlinks --no-progress) or replace the
call inline in the workflow with the equivalent aws s3 sync command targeting
"packaging/releases/$CODENAME" -> "s3://${{ secrets.AWS_S3_BUCKET_RELEASE
}}/$CODENAME" and include the --delete --follow-symlinks --no-progress flags so
behavior matches other syncs.
🪄 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: 3d00e740-8e62-4d96-a767-4d4981fa1b8e
📒 Files selected for processing (2)
.github/workflows/staging-release.yamldockerfiles/Dockerfile.windows
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packaging/sync-apt-repo-to-s3.sh (1)
17-34: Consider enforcing single-writer concurrency for the same S3 prefix.This two-phase publish strategy assumes no concurrent writer for the same
$CODENAMEprefix. A workflow-level concurrency group keyed by codename would reduce race windows and transient repo inconsistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packaging/sync-apt-repo-to-s3.sh` around lines 17 - 34, The script must enforce a single-writer lock per repository codename to avoid concurrent writers; add a per-codename lockfile and use flock to acquire an exclusive non-blocking lock before performing the aws s3 sync and metadata upload steps. Compute a LOCKFILE using the codename variable (e.g., LOCKFILE="/var/lock/sync-apt-${CODENAME}.lock" or derived from DESTINATION/CODENAME), open a file descriptor for it (exec 200>"$LOCKFILE"), attempt to acquire the lock with flock -n 200 || { echo "another publish in progress for $CODENAME"; exit 1; }, and keep the descriptor open for the duration of the script so the lock is held until completion; release happens automatically when the script exits. Ensure the locking code wraps both the aws s3 sync block and the subsequent metadata cp loop so they run under the same exclusive lock.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packaging/sync-apt-repo-to-s3.sh`:
- Around line 17-29: Move the repository preflight checks to run before the
destructive aws s3 sync call: validate that DIST_DIR="$SOURCE_DIR/dists" exists
and any other necessary preconditions (using the existing DIST_DIR and
SOURCE_DIR variables) and exit non-zero if they fail, then run the aws s3 sync
command (which currently uses --delete, --follow-symlinks, --no-progress and
excludes) only after those checks pass; update the script so the check block
that echoes "ERROR: missing dists dir in source: $DIST_DIR" and exits is located
before the aws s3 sync invocation.
- Around line 21-23: The script currently excludes "dists/*/InRelease",
"dists/*/Release", and "dists/*/Release.gpg" from deletion which leaves stale
Release* objects in S3; update the sync logic so metadata is synced with include
filters and deleted when missing: remove or stop excluding those three patterns
from the deletion pass, and add a cleanup sync/pass that uses include patterns
like "dists/*/Release*" (and the specific file names) with --delete (or
equivalent delete-enabled command) so orphaned Release*, InRelease and
Release.gpg objects are removed; search for the rsync/aws s3 sync invocation(s)
around the existing exclude lines and modify them to perform an include+--delete
cleanup for the dists/*/Release* metadata.
---
Nitpick comments:
In `@packaging/sync-apt-repo-to-s3.sh`:
- Around line 17-34: The script must enforce a single-writer lock per repository
codename to avoid concurrent writers; add a per-codename lockfile and use flock
to acquire an exclusive non-blocking lock before performing the aws s3 sync and
metadata upload steps. Compute a LOCKFILE using the codename variable (e.g.,
LOCKFILE="/var/lock/sync-apt-${CODENAME}.lock" or derived from
DESTINATION/CODENAME), open a file descriptor for it (exec 200>"$LOCKFILE"),
attempt to acquire the lock with flock -n 200 || { echo "another publish in
progress for $CODENAME"; exit 1; }, and keep the descriptor open for the
duration of the script so the lock is held until completion; release happens
automatically when the script exits. Ensure the locking code wraps both the aws
s3 sync block and the subsequent metadata cp loop so they run under the same
exclusive lock.
🪄 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: fa25dc29-e23c-43d6-9e74-54bdedc5787e
📒 Files selected for processing (1)
packaging/sync-apt-repo-to-s3.sh
Comment on lines
+17
to
+29
| aws s3 sync "$SOURCE_DIR" "$DESTINATION" \ | ||
| --delete \ | ||
| --follow-symlinks \ | ||
| --no-progress \ | ||
| --exclude "dists/*/InRelease" \ | ||
| --exclude "dists/*/Release" \ | ||
| --exclude "dists/*/Release.gpg" | ||
|
|
||
| DIST_DIR="$SOURCE_DIR/dists" | ||
| if [[ ! -d "$DIST_DIR" ]]; then | ||
| echo "ERROR: missing dists dir in source: $DIST_DIR" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
Move repository preflight checks before the destructive sync step.
Line 17 runs a --delete sync before Line 26 validates that dists exists. If that validation fails, the destination may already be partially mutated.
Suggested fix
SOURCE_DIR=$(realpath "$SOURCE_DIR")
+DIST_DIR="$SOURCE_DIR/dists"
+if [[ ! -d "$DIST_DIR" ]]; then
+ echo "ERROR: missing dists dir in source: $DIST_DIR"
+ exit 1
+fi
+
+mapfile -t METADATA_FILES < <(
+ find "$DIST_DIR" -type f \( -name "InRelease" -o -name "Release" -o -name "Release.gpg" \) | sort
+)
+if [[ ${`#METADATA_FILES`[@]} -eq 0 ]]; then
+ echo "ERROR: no APT metadata files found under: $DIST_DIR"
+ exit 1
+fi
+
aws s3 sync "$SOURCE_DIR" "$DESTINATION" \
--delete \
--follow-symlinks \
--no-progress \
--exclude "dists/*/InRelease" \
--exclude "dists/*/Release" \
--exclude "dists/*/Release.gpg"
-
-DIST_DIR="$SOURCE_DIR/dists"
-if [[ ! -d "$DIST_DIR" ]]; then
- echo "ERROR: missing dists dir in source: $DIST_DIR"
- exit 1
-fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packaging/sync-apt-repo-to-s3.sh` around lines 17 - 29, Move the repository
preflight checks to run before the destructive aws s3 sync call: validate that
DIST_DIR="$SOURCE_DIR/dists" exists and any other necessary preconditions (using
the existing DIST_DIR and SOURCE_DIR variables) and exit non-zero if they fail,
then run the aws s3 sync command (which currently uses --delete,
--follow-symlinks, --no-progress and excludes) only after those checks pass;
update the script so the check block that echoes "ERROR: missing dists dir in
source: $DIST_DIR" and exits is located before the aws s3 sync invocation.
Comment on lines
+21
to
+23
| --exclude "dists/*/InRelease" \ | ||
| --exclude "dists/*/Release" \ | ||
| --exclude "dists/*/Release.gpg" |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In AWS CLI s3 sync, when --deleteis used with--exclude, are excluded objects skipped from deletion?
💡 Result:
Yes, excluded objects are skipped from deletion when using aws s3 sync with both --delete and --exclude.
Citations:
- 1: https://docs.aws.amazon.com/cli/latest/reference/s3/sync.html?highlight=sync
- 2: https://docs.aws.amazon.com/cli/latest/reference/s3/sync.html
🏁 Script executed:
cat -n packaging/sync-apt-repo-to-s3.shRepository: fluent/fluent-bit
Length of output: 1387
Excluded metadata won't be deleted; removed suites leave stale Release* objects in S3.
Phase 1 excludes Release* files from deletion, and Phase 2 only uploads existing metadata files without deleting orphaned ones. If a distribution suite is removed from the source, its metadata remains in S3 indefinitely.
Suggested fix (metadata sync with include filters + delete)
-while IFS= read -r metadata_file; do
- relative_path=${metadata_file#"$SOURCE_DIR"/}
- aws s3 cp "$metadata_file" "$DESTINATION/$relative_path" --no-progress
-done < <(find "$DIST_DIR" -type f \( -name "InRelease" -o -name "Release" -o -name "Release.gpg" \) | sort)
+aws s3 sync "$DIST_DIR" "$DESTINATION/dists" \
+ --delete \
+ --no-progress \
+ --exclude "*" \
+ --include "*/InRelease" \
+ --include "*/Release" \
+ --include "*/Release.gpg"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packaging/sync-apt-repo-to-s3.sh` around lines 21 - 23, The script currently
excludes "dists/*/InRelease", "dists/*/Release", and "dists/*/Release.gpg" from
deletion which leaves stale Release* objects in S3; update the sync logic so
metadata is synced with include filters and deleted when missing: remove or stop
excluding those three patterns from the deletion pass, and add a cleanup
sync/pass that uses include patterns like "dists/*/Release*" (and the specific
file names) with --delete (or equivalent delete-enabled command) so orphaned
Release*, InRelease and Release.gpg objects are removed; search for the
rsync/aws s3 sync invocation(s) around the existing exclude lines and modify
them to perform an include+--delete cleanup for the dists/*/Release* metadata.
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.
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