Kiosk demo #74
Kiosk demo #74
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis change introduces a new kiosk demo under the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Device (Kiosk)
participant Webserver
Note over Device (Kiosk): On boot
Device (Kiosk)->>Systemd: Start graphical.target
Systemd->>kiosk-autologin-setup.service: Run if not configured
kiosk-autologin-setup.service->>configure-kiosk-autologin.sh: Configure GDM autologin
configure-kiosk-autologin.sh-->>Systemd: Mark as configured
Systemd->>GDM: Start Display Manager
GDM->>kiosk user session: Autologin as kiosk
kiosk user session->>gnome-kiosk-script: Start session script
gnome-kiosk-script->>gnome-kiosk-script: Read URL config
gnome-kiosk-script->>Firefox: Launch in kiosk mode with URL
Note over Webserver: Running Python HTTP server on port 8080
User->>Device (Kiosk): Interacts with Firefox in kiosk mode
Firefox->>Webserver: Access content from /var/kiosk/catalog via HTTP
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (8)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (14)
demos/kiosk-demo/bootc/configure-kiosk-autologin.sh (2)
6-9: Consider adding additional error handling.The script checks if the autologin setting already exists before modifying the file, which is good. However, it assumes that the
[daemon]section exists in the GDM configuration file. If this section doesn't exist, thesedcommand might fail silently.- if ! grep -q "AutomaticLoginEnable=True" /etc/gdm/custom.conf; then - sed -i '/^\[daemon\]/aAutomaticLoginEnable=True\nAutomaticLogin=kiosk' /etc/gdm/custom.conf - fi + if ! grep -q "AutomaticLoginEnable=True" /etc/gdm/custom.conf; then + if grep -q "^\[daemon\]" /etc/gdm/custom.conf; then + sed -i '/^\[daemon\]/aAutomaticLoginEnable=True\nAutomaticLogin=kiosk' /etc/gdm/custom.conf + else + echo "[daemon]" >> /etc/gdm/custom.conf + echo "AutomaticLoginEnable=True" >> /etc/gdm/custom.conf + echo "AutomaticLogin=kiosk" >> /etc/gdm/custom.conf + fi + fi
11-17: Verify the kiosk user exists.The script configures autologin for the 'kiosk' user but doesn't verify if the user exists. Consider adding a check to ensure the user exists before proceeding.
+ # Check if kiosk user exists + if ! id -u kiosk >/dev/null 2>&1; then + echo "Error: kiosk user does not exist. Please create the user first." + exit 1 + fi + # Add user session info mkdir -p /var/lib/AccountsService/users cat <<EOF > /var/lib/AccountsService/users/kioskdemos/kiosk-demo/kiosk-script/gnome-kiosk-script (3)
7-9: Unusual configuration file path.The path
/etc/kiosk-mode/bin/url.confis non-standard. Configuration files typically reside directly in/etc/kiosk-mode/rather than in abinsubdirectory. Consider relocating the config file.-CONFIG_FILE="/etc/kiosk-mode/bin/url.conf" +CONFIG_FILE="/etc/kiosk-mode/url.conf"
14-14: Explain the purpose of the 5-second delay.The script includes a hardcoded 5-second sleep, but the purpose isn't clear. Add a comment explaining why this delay is necessary or consider making it configurable.
-sleep 5 +# Wait for display server to be fully ready before launching browser +STARTUP_DELAY=${KIOSK_STARTUP_DELAY:-5} +sleep $STARTUP_DELAY
22-23: Verify Firefox is installed.The script assumes Firefox is available but doesn't check before attempting to launch it.
+# Check if browser is installed +if ! command -v "$BROWSER" &> /dev/null; then + echo "[kiosk] Error: $BROWSER is not installed" + sleep 30 # Give time to read error before restarting + exec "$0" "$@" +fi + echo "[kiosk] Launching: $BROWSER -kiosk $URL" "$BROWSER" -kiosk "$URL"demos/kiosk-demo/webserver/Containerfile (3)
1-2: Base image choice
Usingscratchyields an empty image with no filesystem or runtime. If the intent is only to package the compose file, clarify how this image is consumed. Otherwise consider a minimal base (e.g., Alpine, UBI) to ensure a usable filesystem.
3-3: Organize compose file path
Copyingpodman-compose.yamlto/podman-compose.yamlworks, but for clarity you might place it under a dedicated directory (e.g.,/etc/kiosk-demo/compose.yaml).
5-5: Enhance OCI metadata
The labelappType="compose"is a good start. Consider adding standard OCI labels (org.opencontainers.image.title,org.opencontainers.image.version,org.opencontainers.image.description) for better discoverability.demos/kiosk-demo/webserver/podman-compose.yaml (1)
10-10: Add a restart policy
To improve reliability, consider adding a restart policy (e.g.,restart: unless-stopped) so the service recovers if it crashes.demos/kiosk-demo/README.md (3)
8-9: Fix inline code markup
Use single backticks for inline code (e.g., `quay.io/atraeger/rhel-kiosk-demo`) instead of triple backticks to avoid rendering issues in Markdown.
12-14: Correct code block formatting
The path `/var/kiosk/catalog` is wrapped in triple backticks. Switch to single backticks for inline code or present it as a standalone fenced block for clarity.
17-20: Expand “Kiosk script” section
The README mentions placing thegnome-kiosk-scriptbut omits the autologin service setup. Consider adding instructions on installing thekiosk-autologin-setup.serviceand where to place the shell script.demos/kiosk-demo/bootc/kiosk-autologin-setup.service (2)
3-4: Service ordering refinement
To ensure GDM is configured before it starts, consider addingAfter=display-manager.serviceor tying the service’s install target tographical.target.
11-12: Adjust install target
Since this unit configures GDM (a graphical component), it might be more appropriate underWantedBy=graphical.targetrather thanmulti-user.targetfor clearer intent and ordering.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
demos/kiosk-demo/README.md(1 hunks)demos/kiosk-demo/bootc/Containerfile(1 hunks)demos/kiosk-demo/bootc/configure-kiosk-autologin.sh(1 hunks)demos/kiosk-demo/bootc/kiosk-autologin-setup.service(1 hunks)demos/kiosk-demo/kiosk-script/gnome-kiosk-script(1 hunks)demos/kiosk-demo/webserver/Containerfile(1 hunks)demos/kiosk-demo/webserver/podman-compose.yaml(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Build changed OS images (bootc)
demos/kiosk-demo/bootc/configure-kiosk-autologin.sh
[error] 1-1: Directory 'kiosk-demo/bootc' not found. Command 'pushd kiosk-demo/bootc' failed with exit code 1.
demos/kiosk-demo/bootc/kiosk-autologin-setup.service
[error] 1-1: Directory 'kiosk-demo/bootc' not found. Command 'pushd kiosk-demo/bootc' failed with exit code 1.
demos/kiosk-demo/bootc/Containerfile
[error] 1-1: Directory 'kiosk-demo/bootc' not found. Command 'pushd kiosk-demo/bootc' failed with exit code 1.
🔇 Additional comments (4)
demos/kiosk-demo/bootc/configure-kiosk-autologin.sh (1)
1-3: Script has good basic error handling.Using
set -eensures the script exits on any command failure, which is a good practice for system configuration scripts.🧰 Tools
🪛 GitHub Actions: Build changed OS images (bootc)
[error] 1-1: Directory 'kiosk-demo/bootc' not found. Command 'pushd kiosk-demo/bootc' failed with exit code 1.
demos/kiosk-demo/kiosk-script/gnome-kiosk-script (1)
1-3: Good logging setup.Redirecting all output to a log file and setting error handling with
set -eare good practices for a kiosk script.demos/kiosk-demo/bootc/kiosk-autologin-setup.service (2)
1-5: Verify idempotency marker
The conditionConditionPathExists=!/etc/gdm/kiosk-autologin-configuredrelies on the script creating this file. Confirm thatconfigure-kiosk-autologin.shcreates/etc/gdm/kiosk-autologin-configured; otherwise the service will rerun on every boot.🧰 Tools
🪛 GitHub Actions: Build changed OS images (bootc)
[error] 1-1: Directory 'kiosk-demo/bootc' not found. Command 'pushd kiosk-demo/bootc' failed with exit code 1.
1-13:Details
❓ Verification inconclusive
CI failure: incorrect directory path
The pipeline error showsDirectory 'kiosk-demo/bootc' not found, but this service lives underdemos/kiosk-demo/bootc. Please update your GitHub Actions workflow topushd demos/kiosk-demo/bootc(or adjust the path accordingly) so the build step can locate this directory.
🏁 Script executed:
#!/bin/bash # List the kiosk-demo directory structure echo "Directory tree under demos/kiosk-demo:" ls -R demos/kiosk-demo # Search GitHub Actions workflows for any references to kiosk-demo/bootc echo -e "\nSearching workflows for 'kiosk-demo/bootc':" rg -n "kiosk-demo/bootc" -A2 -B2 .github/workflowsLength of output: 540
🏁 Script executed:
#!/bin/bash # Search for directory change commands in GitHub Actions workflows echo "Scanning .github/workflows for cd or pushd usage:" rg -nE '(cd|pushd)' -A2 -B2 .github/workflowsLength of output: 204
CI failure: incorrect directory path in workflow
I couldn’t find any references tokiosk-demo/bootc(orpushd/cdinto it) in your.github/workflowsfiles. However, the error “Directory ‘kiosk-demo/bootc’ not found” suggests the workflow is pointing at the wrong location. The service actually lives under:
- demos/kiosk-demo/bootc/
Please verify the build step in your GitHub Actions workflow (e.g. any
working-directory,cdorpushdcommands) and update the path to:demos/kiosk-demo/bootcso the step can locate this directory.
🧰 Tools
🪛 GitHub Actions: Build changed OS images (bootc)
[error] 1-1: Directory 'kiosk-demo/bootc' not found. Command 'pushd kiosk-demo/bootc' failed with exit code 1.
| # Marker file to ensure this only runs once | ||
| touch /etc/gdm/kiosk-autologin-configured |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Pipeline failure indicates directory structure issue.
The GitHub Actions pipeline is reporting that the directory 'kiosk-demo/bootc' was not found. Ensure that the directory structure matches what's expected in the CI/CD pipeline.
🏁 Script executed:
#!/bin/bash
# Check if the directory structure is correct
ls -la demos/kiosk-demo/
echo "Current directory structure:"
find demos/kiosk-demo -type d | sortLength of output: 569
CI path reference is incorrect
The bootc directory does exist under demos/kiosk-demo/bootc, but your GitHub Actions job is looking for kiosk-demo/bootc. Update the workflow (e.g. in .github/workflows/…) to point at the correct path:
- Current (failing) path:
kiosk-demo/bootc/configure-kiosk-autologin.sh - Correct path:
demos/kiosk-demo/bootc/configure-kiosk-autologin.sh
This will resolve the “directory not found” errors in your pipeline.
| if [[ -f "$CONFIG_FILE" ]]; then | ||
| URL="$(cat "$CONFIG_FILE")" | ||
| else | ||
| URL="$DEFAULT_URL" | ||
| fi |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for the URL from config file.
The script reads the URL from a config file without validation. If the file exists but is empty or contains an invalid URL, this could cause issues.
if [[ -f "$CONFIG_FILE" ]]; then
URL="$(cat "$CONFIG_FILE")"
+ # Validate URL format
+ if [[ ! "$URL" =~ ^https?:// ]]; then
+ echo "[kiosk] Warning: Invalid URL in config file, using default"
+ URL="$DEFAULT_URL"
+ fi
else
URL="$DEFAULT_URL"
fi| sleep 1.0 | ||
| exec "$0" "$@" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement graceful exit mechanism.
The script restarts infinitely with no way to exit gracefully. Consider adding a mechanism to detect a deliberate shutdown or a maximum restart count to prevent excessive restarts on persistent failures.
-sleep 1.0
-exec "$0" "$@"
+# Create tracking file for restart count
+RESTART_COUNT_FILE="/tmp/kiosk-restart-count"
+MAX_RESTARTS=10
+
+# Check if this is a new session or continuation
+if [[ ! -f "$RESTART_COUNT_FILE" ]]; then
+ echo "1" > "$RESTART_COUNT_FILE"
+else
+ CURRENT_COUNT=$(cat "$RESTART_COUNT_FILE")
+ if [[ "$CURRENT_COUNT" -ge "$MAX_RESTARTS" ]]; then
+ echo "[kiosk] Maximum restart count ($MAX_RESTARTS) reached. Exiting..."
+ exit 1
+ fi
+ echo $((CURRENT_COUNT + 1)) > "$RESTART_COUNT_FILE"
+fi
+
+sleep 1.0
+exec "$0" "$@"| working_dir: /app | ||
| command: ["python", "-m", "http.server", "8080"] | ||
| volumes: | ||
| - /var/kiosk/catalog:/app:ro,z |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Run server as non-root
By default the Python HTTP server runs as root. For better security, add a non-root user and specify user: nobody (or a dedicated UID/GID) in this service, or build a custom image with a non-root user.
| version: "3.8" | ||
| services: | ||
| kiosk: | ||
| image: docker.io/library/python:3.11-slim |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Pin container image to a digest
Relying on python:3.11-slim can introduce drift. Pin to a specific digest (e.g. python@sha256:...) to guarantee reproducible builds.
avishayt
force-pushed
the
kiosk
branch
2 times, most recently
from
59bf8d6 to
c662df6
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
demos/kiosk-demo/deployment/fleet.yaml (2)
34-39: Normalize indentation forkiosk-contentgitRef block.Ensure all keys under
gitRef:are indented uniformly (two spaces deeper than the sequence marker) to maintain a valid mapping:- - gitRef: - name: kiosk-content - mountPath: /var/kiosk/catalog - path: / - repository: rh-kiosk - targetRevision: 'content-{{ .metadata.labels.region }}' + - gitRef: + name: kiosk-content + mountPath: /var/kiosk/catalog + path: / + repository: rh-kiosk + targetRevision: 'content-{{ .metadata.labels.region }}'
40-45: Normalize indentation forgnome-kiosk-scriptgitRef block.Similarly, align the
gnome-kiosk-scriptentries undergitRef:for consistency and correct YAML parsing:- - gitRef: - name: gnome-kiosk-script - mountPath: /home/kiosk/.local/bin/ - path: /demos/kiosk-demo/kiosk-script/gnome-kiosk-script - repository: flightctl-demos - targetRevision: kiosk + - gitRef: + name: gnome-kiosk-script + mountPath: /home/kiosk/.local/bin/ + path: /demos/kiosk-demo/kiosk-script/gnome-kiosk-script + repository: flightctl-demos + targetRevision: kiosk
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
demos/kiosk-demo/README.md(1 hunks)demos/kiosk-demo/bootc/Containerfile(1 hunks)demos/kiosk-demo/bootc/configure-kiosk-autologin.sh(1 hunks)demos/kiosk-demo/bootc/kiosk-autologin-setup.service(1 hunks)demos/kiosk-demo/deployment/fleet.yaml(1 hunks)demos/kiosk-demo/kiosk-script/gnome-kiosk-script(1 hunks)demos/kiosk-demo/webserver/Containerfile(1 hunks)demos/kiosk-demo/webserver/podman-compose.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- demos/kiosk-demo/bootc/Containerfile
- demos/kiosk-demo/bootc/configure-kiosk-autologin.sh
🚧 Files skipped from review as they are similar to previous changes (5)
- demos/kiosk-demo/webserver/Containerfile
- demos/kiosk-demo/README.md
- demos/kiosk-demo/kiosk-script/gnome-kiosk-script
- demos/kiosk-demo/bootc/kiosk-autologin-setup.service
- demos/kiosk-demo/webserver/podman-compose.yaml
🧰 Additional context used
🪛 YAMLlint (1.35.1)
demos/kiosk-demo/deployment/fleet.yaml
[error] 21-21: syntax error: expected , but found '-'
(syntax)
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
demos/kiosk-demo/deployment/fleet.yaml (2)
19-29:⚠️ Potential issueFix YAML syntax error in
gdm-restart-hookinline config.The
- content:line is mis-indented and erroneously treated as a list item, causing a YAML syntax error (expected <block end>, but found '-'). This was flagged previously in an earlier review and needs correction.- - inline: - name: gdm-restart-hook - - content: |- - - run: /usr/bin/systemctl restart gdm - timeout: 10s - if: - - path: /etc/kiosk-mode/bin/url.conf - op: [created, updated] - - path: /var/kiosk/catalog/ - op: [created, updated, deleted] - path: /etc/flightctl/hooks.d/afterupdating/10-restart-gdm.yaml + - inline: + name: gdm-restart-hook + content: |- + - run: /usr/bin/systemctl restart gdm + timeout: 10s + if: + - path: /etc/kiosk-mode/bin/url.conf + op: [created, updated] + - path: /var/kiosk/catalog/ + op: [created, updated, deleted] + path: /etc/flightctl/hooks.d/afterupdating/10-restart-gdm.yaml🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 21-21: syntax error: expected , but found '-'
(syntax)
30-33:⚠️ Potential issueFix YAML syntax error in
kiosk-urlinline config.The same indentation issue appears in the
kiosk-urlblock (misplaced dash beforecontent). Align all fields underinline:as a mapping entry, not a sequence.- - inline: - name: kiosk-url - - content: http://localhost:8080 - path: /etc/kiosk-mode/bin/url.conf + - inline: + name: kiosk-url + content: http://localhost:8080 + path: /etc/kiosk-mode/bin/url.conf
🧹 Nitpick comments (1)
demos/kiosk-demo/deployment/fleet.yaml (1)
15-17: Optional: Reorder fields underapplicationsfor consistency.For readability, consider grouping
name, thenimage, followed byenvVars, matching common Kubernetes spec patterns.- - envVars: {} - image: quay.io/atraeger/kiosk-app:v3 - name: kiosk-app + - name: kiosk-app + image: quay.io/atraeger/kiosk-app:v3 + envVars: {}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
demos/kiosk-demo/README.md(1 hunks)demos/kiosk-demo/bootc/Containerfile(1 hunks)demos/kiosk-demo/bootc/configure-kiosk-autologin.sh(1 hunks)demos/kiosk-demo/bootc/kiosk-autologin-setup.service(1 hunks)demos/kiosk-demo/deployment/fleet.yaml(1 hunks)demos/kiosk-demo/kiosk-script/gnome-kiosk-script(1 hunks)demos/kiosk-demo/webserver/Containerfile(1 hunks)demos/kiosk-demo/webserver/podman-compose.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- demos/kiosk-demo/README.md
- demos/kiosk-demo/bootc/configure-kiosk-autologin.sh
🚧 Files skipped from review as they are similar to previous changes (5)
- demos/kiosk-demo/webserver/Containerfile
- demos/kiosk-demo/kiosk-script/gnome-kiosk-script
- demos/kiosk-demo/webserver/podman-compose.yaml
- demos/kiosk-demo/bootc/kiosk-autologin-setup.service
- demos/kiosk-demo/bootc/Containerfile
🧰 Additional context used
🪛 YAMLlint (1.35.1)
demos/kiosk-demo/deployment/fleet.yaml
[error] 21-21: syntax error: expected , but found '-'
(syntax)
| - gitRef: | ||
| name: kiosk-content | ||
| mountPath: /var/kiosk/catalog | ||
| path: /'content-{{ .metadata.labels.region }}' | ||
| repository: rh-kiosk | ||
| targetRevision: summit |
There was a problem hiding this comment.
Correct path quoting and indentation in kiosk-content gitRef.
The path: /'content-{{ .metadata.labels.region }}' includes extraneous slashes and quotes, which will be interpreted literally rather than templated. Additionally, ensure consistent indentation under - gitRef:.
- - gitRef:
- name: kiosk-content
- mountPath: /var/kiosk/catalog
- path: /'content-{{ .metadata.labels.region }}'
- repository: rh-kiosk
- targetRevision: summit
+ - gitRef:
+ name: kiosk-content
+ mountPath: /var/kiosk/catalog
+ path: content-{{ .metadata.labels.region }}
+ repository: rh-kiosk
+ targetRevision: summit📝 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.
| - gitRef: | |
| name: kiosk-content | |
| mountPath: /var/kiosk/catalog | |
| path: /'content-{{ .metadata.labels.region }}' | |
| repository: rh-kiosk | |
| targetRevision: summit | |
| - gitRef: | |
| name: kiosk-content | |
| mountPath: /var/kiosk/catalog | |
| path: content-{{ .metadata.labels.region }} | |
| repository: rh-kiosk | |
| targetRevision: summit |
| - gitRef: | ||
| name: gnome-kiosk-script | ||
| mountPath: /home/kiosk/.local/bin/ | ||
| path: /demos/kiosk-demo/kiosk-script/gnome-kiosk-script | ||
| repository: flightctl-demos | ||
| targetRevision: kiosk |
There was a problem hiding this comment.
Standardize path for gnome-kiosk-script gitRef to relative notation.
The path should be relative to the repository root without a leading slash, and the indentation for mapping entries under - gitRef: should match other blocks.
- - gitRef:
- name: gnome-kiosk-script
- mountPath: /home/kiosk/.local/bin/
- path: /demos/kiosk-demo/kiosk-script/gnome-kiosk-script
- repository: flightctl-demos
- targetRevision: kiosk
+ - gitRef:
+ name: gnome-kiosk-script
+ mountPath: /home/kiosk/.local/bin/
+ path: demos/kiosk-demo/kiosk-script/gnome-kiosk-script
+ repository: flightctl-demos
+ targetRevision: kiosk📝 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.
| - gitRef: | |
| name: gnome-kiosk-script | |
| mountPath: /home/kiosk/.local/bin/ | |
| path: /demos/kiosk-demo/kiosk-script/gnome-kiosk-script | |
| repository: flightctl-demos | |
| targetRevision: kiosk | |
| - gitRef: | |
| name: gnome-kiosk-script | |
| mountPath: /home/kiosk/.local/bin/ | |
| path: demos/kiosk-demo/kiosk-script/gnome-kiosk-script | |
| repository: flightctl-demos | |
| targetRevision: kiosk |
1 participant
Summary by CodeRabbit
New Features
Documentation