Skip to content

Conversation

@GordonFreemanK
Copy link

@GordonFreemanK GordonFreemanK commented Oct 23, 2025

I tried using the ghcr.io/acoustid/acoustid-index:main image, and I had a file not found error in the logs. I checked and fpindex wasn't executable.

I checked that it was indeed the issue with this simple Dockerfile which works:

FROM ghcr.io/acoustid/acoustid-index:main
USER root
RUN chmod +x /usr/bin/fpindex
USER acoustid

So the change in this PR should fix this issue, but there might be a cleaner solution upstream fixing the zig build which should have added the permission itself.

I tried using the ghcr.io/acoustid/acoustid-index:main image, and I had a file not found error. I checked and fpindex wasn't executable.

I checked that it was indeed the issue with this simple Dockerfile which works:
```
FROM ghcr.io/acoustid/acoustid-index:main
USER root
RUN chmod +x /usr/bin/fpindex
USER acoustid
```

So this change should fix this issue, but there might be a cleaner solution upstream fixing the zig build which should have added the permission itself.
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 23, 2025

Walkthrough

Final Docker stage adjusted to copy the built fpindex binary into /usr/bin using ADD --chmod=755, ensuring the installed binary is executable. No other behavior or API changes.

Changes

Cohort / File(s) Summary
Dockerfile permission change
Dockerfile
Replaced final-stage COPY/ADD with ADD --chmod=755 when placing zig-out/bin/fpindex into /usr/bin, so the binary has executable permissions after the image build.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The title "Make fpindex executable" directly describes the core change made in the Dockerfile—adding --chmod=755 to the binary when copying it to /usr/bin to ensure it has executable permissions. The title is concise, specific, and leaves no ambiguity about what problem is being addressed. Someone reviewing the commit history would immediately understand the intent without needing to dig into the details.
Description Check ✅ Passed The description clearly relates to the changeset by explaining the problem (fpindex wasn't executable causing "file not found" errors), providing a working Dockerfile workaround that validates the fix, and contextualizing why the PR change should resolve the issue. It contains specific technical details and reasoning rather than vague language, making it easy to understand the motivation and scope of the change.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3b3147b and 69a6618.

📒 Files selected for processing (1)
  • Dockerfile (1 hunks)
🧰 Additional context used
🪛 Hadolint (2.14.0)
Dockerfile

[error] 19-19: unexpected '-'
expecting '', at least one space, or only the --checksum, --chown, --chmod, --link, --exclude flags or the src and dest paths

(DL1000)

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
Dockerfile (1)

19-19: Prefer COPY over ADD for local files.

The syntax fix to --chmod=755 is correct. However, Hadolint flags this because ADD is overkill here—it's designed for remote URLs and auto-extraction. For copying a local file, use COPY --chmod=755 instead.

-ADD --chmod=755 zig-out/bin/fpindex /usr/bin
+COPY --chmod=755 zig-out/bin/fpindex /usr/bin

This resolves the DL3020 linting hint and better conveys intent.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 69a6618 and 01fbaf5.

📒 Files selected for processing (1)
  • Dockerfile (1 hunks)
🧰 Additional context used
🪛 Hadolint (2.14.0)
Dockerfile

[error] 19-19: Use COPY instead of ADD for files and folders

(DL3020)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant