Documentation#28
Merged
Merged
Conversation
added 3 commits
May 12, 2026 14:55
Signed-off-by: Marek Safarik <msafarik@redhat.com>
Signed-off-by: Marek Safarik <msafarik@redhat.com>
Signed-off-by: Marek Safarik <msafarik@redhat.com>
Reviewer's GuideDocumentation-focused PR that rewrites the README to add configuration and Makefile command documentation, introduces architecture and MCP tools reference docs, and adjusts Packit/e2e configs to run functional tests across additional Fedora targets. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughUpdates example configuration defaults (Keylime API v2.5, TLS server name localhost, port 3000), migrates FMF refs to a TMT dynamic reference, adds Fedora “latest” targets to Packit CI, and adds Makefile targets and systemd wiring for certificate ACL setup and cleanup. ChangesConfiguration and Test Infrastructure
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
![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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@e2e/provision/setup-keylime-services/main.fmf`:
- Line 13: The five FMF files reference a missing .tmt/dynamic_ref.fmf; either
create the .tmt directory and add a .tmt/dynamic_ref.fmf with the expected ref
configuration, or update the ref entries in the affected files (main.fmf in
e2e/provision/setup-keylime-services, e2e/provision/cleanup-keylime-services,
and the three plan files e2e/plans/keylime-mcp-main.fmf, keylime-mcp-server.fmf,
keylime-mcp-client.fmf) to point to an existing FMF reference path; ensure the
created .tmt/dynamic_ref.fmf exports the same variables/refs those five files
expect so tests resolve correctly.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 38ca73af-f88a-4218-9259-fca35806a9ce
⛔ Files ignored due to path filters (3)
README.mdis excluded by!**/*.mddoc/architecture.mdis excluded by!**/*.mddoc/tools.mdis excluded by!**/*.md
📒 Files selected for processing (7)
.env.example.packit.yamle2e/plans/keylime-mcp-client.fmfe2e/plans/keylime-mcp-main.fmfe2e/plans/keylime-mcp-server.fmfe2e/provision/cleanup-keylime-services/main.fmfe2e/provision/setup-keylime-services/main.fmf
Signed-off-by: Marek Safarik <msafarik@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Makefile (1)
73-85:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPreflight new command dependencies in
check-deps.Lines 73–85 don't validate
setfacl/sudo/systemctl, even though Line 46 and Line 64 depend on them. Missing tools will fail later with less actionable errors.🤖 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 `@Makefile` around lines 73 - 85, The check-deps Makefile target currently only verifies Go and certificate files but omits checks for runtime tools used elsewhere; update the check-deps recipe (the check-deps target) to explicitly validate availability of setfacl, sudo, and systemctl (using command -v or similar) and emit clear error messages with remediation hints if any are missing so the build fails fast with actionable guidance; keep the existing CERT_FILES/KEYLIME_CERT_DIR certificate checks and add these tool checks before any certificate or setup steps that rely on them.
🤖 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 `@Makefile`:
- Around line 46-71: The ACL commands in targets setup-certs and
setup-certs-session grant permissions to $(USER), which becomes root under sudo;
change occurrences to an expression that prefers the original invoking user
(SUDO_USER) falling back to USER (e.g. use $(or $(SUDO_USER),$(USER))) and
update every place it appears (the service unit lines created in setup-certs
that include ExecStart entries and the setfacl loops in setup-certs-session) so
setfacl and the unit file grant ACLs to the effective non-root user; ensure
references to CERT_FILES, KEYLIME_CERT_DIR and SYSTEMD_SERVICE remain unchanged
except for replacing $(USER) with the SUDO_USER-or-USER expression.
---
Outside diff comments:
In `@Makefile`:
- Around line 73-85: The check-deps Makefile target currently only verifies Go
and certificate files but omits checks for runtime tools used elsewhere; update
the check-deps recipe (the check-deps target) to explicitly validate
availability of setfacl, sudo, and systemctl (using command -v or similar) and
emit clear error messages with remediation hints if any are missing so the build
fails fast with actionable guidance; keep the existing
CERT_FILES/KEYLIME_CERT_DIR certificate checks and add these tool checks before
any certificate or setup steps that rely on them.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 04efe6d3-4ccb-489c-8fb1-00d74d7d22cf
⛔ Files ignored due to path filters (2)
README.mdis excluded by!**/*.mddoc/tools.mdis excluded by!**/*.md
📒 Files selected for processing (1)
Makefile
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@Makefile`:
- Around line 78-81: The help text for the Makefile's dependency check is out of
sync with the expanded checks; update the Makefile help text (the description
shown for the check-deps/check targets) so it lists setfacl, systemctl, and sudo
alongside the existing Go/cert checks, keeping the alignment/format of the help
output consistent with the other lines; locate the help/usage echo block and the
check-deps target (the block that currently runs the three `@command` -v ...
checks) and expand its descriptive line to mention all four dependencies.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
…t ai PR review) Signed-off-by: Marek Safarik <msafarik@redhat.com>
sergio-correia
requested changes
May 15, 2026
| @echo " Certs: OK" | ||
| @echo "All dependencies satisfied." | ||
|
|
||
| install: setup-certs check-deps .env build |
Collaborator
There was a problem hiding this comment.
Not from this PR, but this caught my attention anyway. Shouldn't we have check-deps before setup-certs?
Comment on lines
+49
to
+59
| @printf '%s\n' \ | ||
| "[Unit]" \ | ||
| "Description=Grant certificate access for Keylime MCP" \ | ||
| "After=systemd-tmpfiles-setup.service" \ | ||
| "[Service]" \ | ||
| "Type=oneshot" \ | ||
| $(foreach f,$(CERT_FILES),"ExecStart=/usr/bin/setfacl -m u:$(EFFECTIVE_USER):r $(KEYLIME_CERT_DIR)/$(f)") \ | ||
| "ExecStart=/usr/bin/setfacl -m u:$(EFFECTIVE_USER):rx /var/lib/keylime" \ | ||
| "ExecStart=/usr/bin/setfacl -m u:$(EFFECTIVE_USER):rx $(KEYLIME_CERT_DIR)" \ | ||
| "[Install]" \ | ||
| "WantedBy=multi-user.target" \ |
Collaborator
There was a problem hiding this comment.
I think we need RemainAfterExit=yes. Without it, the service shows as inactive (dead) after running, even on success. With it, it shows active (exited), which is kind of the standard pattern for oneshot setup services, and the only way to confirm via systemctl that it ran on the current boot. Add it after "Type=oneshot".
Comment on lines
+55
to
+57
| $(foreach f,$(CERT_FILES),"ExecStart=/usr/bin/setfacl -m u:$(EFFECTIVE_USER):r $(KEYLIME_CERT_DIR)/$(f)") \ | ||
| "ExecStart=/usr/bin/setfacl -m u:$(EFFECTIVE_USER):rx /var/lib/keylime" \ | ||
| "ExecStart=/usr/bin/setfacl -m u:$(EFFECTIVE_USER):rx $(KEYLIME_CERT_DIR)" \ |
Collaborator
There was a problem hiding this comment.
Let's change the order of the ExecStart lines here: do the directories first, then the files. The way it is works because the services run as root, but it creates a brief window during boot where file ACLs exist but directories are not traversable.
Signed-off-by: Marek Safarik <msafarik@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@Makefile`:
- Line 93: The install target fails because check-deps runs the certificate
readability checks (in the check-deps target) before setup-certs can grant ACLs;
fix by either (A) reordering install's prerequisites to run setup-certs before
check-deps (change install: check-deps setup-certs .env build to install:
setup-certs check-deps .env build) or (B) add a new lightweight prerequisite
(e.g., check-deps-tools) that omits the cert-readability checks and use that in
install instead of check-deps; update the Makefile so install invokes
setup-certs (or the tools-only check) prior to any cert validation to avoid
early exit on fresh systems while preserving the full check-deps target for
CI/dev flows.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
…ore its checked Signed-off-by: Marek Safarik <msafarik@redhat.com>
Signed-off-by: Marek Safarik <msafarik@redhat.com>
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.
Rewrite README to include configuration table and detailed Makefile descriptions. Add an architecture overview with a Mermaid component diagram and a project structure diagram. Add the MCP tools reference listing all 23 tools and fix configs for functional e2e tests.
Summary by Sourcery
Document Keylime MCP architecture, configuration, and tooling, and update testing and CI settings.
CI:
Documentation:
Tests:
Summary by CodeRabbit
Chores
Tests