fix: installer package install, resolver handling, systemd RW paths; … #17
Conversation
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
Leitfaden für ReviewerDieser PR überarbeitet das Installationsskript, um eine robuste DNS-Resolver-Behandlung (Backup, Fallback, Finalisierung) zu implementieren, die Verwaltung von Umgebungsvariablen für die Python-Suite zu optimieren (API-Schlüssel- und Log-Level-Persistenz), die Sicherheit des systemd-Dienstes zu erhöhen, die Erkennung von Portkonflikten zu verbessern und die Paket- und Konfigurationsverwaltung aufzuräumen. Sequenzdiagramm für verbesserte DNS-Resolver-Behandlung während der InstallationsequenceDiagram
participant Installer
participant Systemd
participant OS
participant ResolvConf
Installer->>Systemd: Check if systemd-resolved is active
alt systemd-resolved is active
Installer->>Systemd: Stop systemd-resolved
Installer->>Systemd: Disable systemd-resolved
Installer->>ResolvConf: Backup /etc/resolv.conf if symlink
Installer->>ResolvConf: Write fallback resolvers (1.1.1.1, 9.9.9.9)
end
Installer->>ResolvConf: After Pi-hole install, point resolver to 127.0.0.1 and fallbacks
Installer->>OS: Success message: Resolver prepared and finalized
Änderungen auf Dateiebene
Tipps und BefehleInteraktion mit Sourcery
Ihre Erfahrung anpassenGreifen Sie auf Ihr Dashboard zu, um:
Hilfe erhalten
Original review guide in EnglishReviewer's GuideThis PR refactors the installer script to implement robust DNS resolver handling (backup, fallback, finalization), streamlines environment variable management for the Python suite (API key and log level persistence), strengthens systemd service security, enhances port conflict detection, and tidies up package and configuration handling. Sequence diagram for improved DNS resolver handling during installationsequenceDiagram
participant Installer
participant Systemd
participant OS
participant ResolvConf
Installer->>Systemd: Check if systemd-resolved is active
alt systemd-resolved is active
Installer->>Systemd: Stop systemd-resolved
Installer->>Systemd: Disable systemd-resolved
Installer->>ResolvConf: Backup /etc/resolv.conf if symlink
Installer->>ResolvConf: Write fallback resolvers (1.1.1.1, 9.9.9.9)
end
Installer->>ResolvConf: After Pi-hole install, point resolver to 127.0.0.1 and fallbacks
Installer->>OS: Success message: Resolver prepared and finalized
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hallo – ich habe Ihre Änderungen überprüft – hier ist etwas Feedback:
- Stellen Sie sicher, dass der pihole-suite systemd-Dienst die generierte .env-Datei tatsächlich lädt (z. B. über EnvironmentFile=), damit SUITE_API_KEY und SUITE_LOG_LEVEL dem Dienst zur Verfügung stehen.
- Fügen Sie eine anfängliche Berechtigungsprüfung in install.sh hinzu, um schnell einen Fehler zu melden, wenn das Skript nicht mit ausreichenden Berechtigungen zum Schreiben nach /etc oder zur Verwaltung von systemd-Einheiten ausgeführt wird.
- Bewahren Sie in write_resolv_conf() die ursprünglichen Eigentumsverhältnisse und Berechtigungen von /etc/resolv.conf und überprüfen Sie, ob der Schreibvorgang erfolgreich war, um zu vermeiden, dass die Resolver-Datei in einem ungültigen Zustand zurückbleibt.
Prompt für KI-Agenten
Bitte gehen Sie auf die Kommentare aus dieser Codeüberprüfung ein:
## Allgemeine Kommentare
- Stellen Sie sicher, dass der pihole-suite systemd-Dienst die generierte .env-Datei tatsächlich lädt (z. B. über EnvironmentFile=), damit SUITE_API_KEY und SUITE_LOG_LEVEL dem Dienst zur Verfügung stehen.
- Fügen Sie eine anfängliche Berechtigungsprüfung in install.sh hinzu, um schnell einen Fehler zu melden, wenn das Skript nicht mit ausreichenden Berechtigungen zum Schreiben nach /etc oder zur Verwaltung von systemd-Einheiten ausgeführt wird.
- Bewahren Sie in write_resolv_conf() die ursprünglichen Eigentumsverhältnisse und Berechtigungen von /etc/resolv.conf und überprüfen Sie, ob der Schreibvorgang erfolgreich war, um zu vermeiden, dass die Resolver-Datei in einem ungültigen Zustand zurückbleibt.
## Einzelne Kommentare
### Kommentar 1
<location> `install.sh:132` </location>
<code_context>
curl -fsSL https://www.internic.net/domain/named.root -o /var/lib/unbound/root.hints
- cat > /etc/unbound/unbound.conf.d/pi-hole.conf <<EOF
+ cat > /etc/unbound/unbound.conf.d/pi-hole.conf <<'UNBOUND_EOF'
server:
interface: 127.0.0.1
</code_context>
<issue_to_address>
**question:** Das Umschalten auf ein einfach zitiertes Heredoc deaktiviert die Variablenerweiterung.
Wenn Sie möchten, dass Variablen wie $UNBOUND_PORT erweitert werden, verwenden Sie einen nicht zitierten Heredoc-Begrenzer. Andernfalls stellen Sie sicher, dass alle Werte fest codiert sind.
</issue_to_address>
### Kommentar 2
<location> `install.sh:208-215` </location>
<code_context>
sudo -u "$INSTALL_USER" .venv/bin/python scripts/bootstrap.py || true
- cat > /etc/systemd/system/pihole-suite.service <<EOF
+ local env_file="$PROJECT_DIR/.env"
+ local suite_api_key="$SUITE_API_KEY_ENV"
+ if [[ -z "$suite_api_key" ]]; then
+ suite_api_key="$(extract_env_value "SUITE_API_KEY" "$env_file")"
+ fi
+ if [[ -z "$suite_api_key" ]]; then
+ suite_api_key="$(openssl rand -hex 16)"
+ fi
+
+ cat > "$env_file" <<ENV
+SUITE_API_KEY=$suite_api_key
+SUITE_PORT=$PYTHON_SUITE_PORT
+SUITE_DATA_DIR=$PROJECT_DIR/data
+SUITE_LOG_LEVEL=$SUITE_LOG_LEVEL_ENV
+ENV
+ chown "$INSTALL_USER:$INSTALL_USER" "$env_file"
+
+ cat > /etc/systemd/system/pihole-suite.service <<SERVICE_EOF
</code_context>
<issue_to_address>
**suggestion:** Mehrere Quellen für SUITE_API_KEY können zu Verwirrung führen.
Bitte klären oder dokumentieren Sie die Rangfolge der SUITE_API_KEY-Zuweisung, um Verwirrung zu vermeiden, wenn sich Umgebungs- und .env-Werte unterscheiden.
```suggestion
# SUITE_API_KEY assignment order of precedence:
# 1. If SUITE_API_KEY_ENV is set (from environment), use it.
# 2. Else, if SUITE_API_KEY is set in .env file, use that.
# 3. Else, generate a random value.
local env_file="$PROJECT_DIR/.env"
local suite_api_key="$SUITE_API_KEY_ENV"
if [[ -z "$suite_api_key" ]]; then
suite_api_key="$(extract_env_value "SUITE_API_KEY" "$env_file")"
fi
if [[ -z "$suite_api_key" ]]; then
suite_api_key="$(openssl rand -hex 16)"
fi
```
</issue_to_address>Sourcery ist kostenlos für Open Source – wenn Ihnen unsere Bewertungen gefallen, denken Sie bitte darüber nach, sie zu teilen ✨
Original comment in English
Hey there - I've reviewed your changes - here's some feedback:
- Ensure the pihole-suite systemd service actually loads the generated .env file (e.g. via EnvironmentFile=) so SUITE_API_KEY and SUITE_LOG_LEVEL are available to the service.
- Add an initial privilege check in install.sh to fail fast if the script isn’t running with sufficient permissions to write to /etc or manage systemd units.
- In write_resolv_conf(), preserve the original /etc/resolv.conf ownership and permissions and verify the write succeeded to avoid leaving the resolver file in an invalid state.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Ensure the pihole-suite systemd service actually loads the generated .env file (e.g. via EnvironmentFile=) so SUITE_API_KEY and SUITE_LOG_LEVEL are available to the service.
- Add an initial privilege check in install.sh to fail fast if the script isn’t running with sufficient permissions to write to /etc or manage systemd units.
- In write_resolv_conf(), preserve the original /etc/resolv.conf ownership and permissions and verify the write succeeded to avoid leaving the resolver file in an invalid state.
## Individual Comments
### Comment 1
<location> `install.sh:132` </location>
<code_context>
curl -fsSL https://www.internic.net/domain/named.root -o /var/lib/unbound/root.hints
- cat > /etc/unbound/unbound.conf.d/pi-hole.conf <<EOF
+ cat > /etc/unbound/unbound.conf.d/pi-hole.conf <<'UNBOUND_EOF'
server:
interface: 127.0.0.1
</code_context>
<issue_to_address>
**question:** Switching to single-quoted heredoc disables variable expansion.
If you want variables like $UNBOUND_PORT to be expanded, use an unquoted heredoc delimiter. Otherwise, ensure all values are hardcoded.
</issue_to_address>
### Comment 2
<location> `install.sh:208-215` </location>
<code_context>
sudo -u "$INSTALL_USER" .venv/bin/python scripts/bootstrap.py || true
- cat > /etc/systemd/system/pihole-suite.service <<EOF
+ local env_file="$PROJECT_DIR/.env"
+ local suite_api_key="$SUITE_API_KEY_ENV"
+ if [[ -z "$suite_api_key" ]]; then
+ suite_api_key="$(extract_env_value "SUITE_API_KEY" "$env_file")"
+ fi
+ if [[ -z "$suite_api_key" ]]; then
+ suite_api_key="$(openssl rand -hex 16)"
+ fi
+
+ cat > "$env_file" <<ENV
+SUITE_API_KEY=$suite_api_key
+SUITE_PORT=$PYTHON_SUITE_PORT
+SUITE_DATA_DIR=$PROJECT_DIR/data
+SUITE_LOG_LEVEL=$SUITE_LOG_LEVEL_ENV
+ENV
+ chown "$INSTALL_USER:$INSTALL_USER" "$env_file"
+
+ cat > /etc/systemd/system/pihole-suite.service <<SERVICE_EOF
</code_context>
<issue_to_address>
**suggestion:** Multiple sources for SUITE_API_KEY may cause confusion.
Please clarify or document the order of precedence for SUITE_API_KEY assignment to prevent confusion if environment and .env values differ.
```suggestion
# SUITE_API_KEY assignment order of precedence:
# 1. If SUITE_API_KEY_ENV is set (from environment), use it.
# 2. Else, if SUITE_API_KEY is set in .env file, use that.
# 3. Else, generate a random value.
local env_file="$PROJECT_DIR/.env"
local suite_api_key="$SUITE_API_KEY_ENV"
if [[ -z "$suite_api_key" ]]; then
suite_api_key="$(extract_env_value "SUITE_API_KEY" "$env_file")"
fi
if [[ -z "$suite_api_key" ]]; then
suite_api_key="$(openssl rand -hex 16)"
fi
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
install.sh
Outdated
| curl -fsSL https://www.internic.net/domain/named.root -o /var/lib/unbound/root.hints | ||
|
|
||
| cat > /etc/unbound/unbound.conf.d/pi-hole.conf <<EOF | ||
| cat > /etc/unbound/unbound.conf.d/pi-hole.conf <<'UNBOUND_EOF' |
There was a problem hiding this comment.
question: Das Umschalten auf ein einfach zitiertes Heredoc deaktiviert die Variablenerweiterung.
Wenn Sie möchten, dass Variablen wie $UNBOUND_PORT erweitert werden, verwenden Sie einen nicht zitierten Heredoc-Begrenzer. Andernfalls stellen Sie sicher, dass alle Werte fest codiert sind.
Original comment in English
question: Switching to single-quoted heredoc disables variable expansion.
If you want variables like $UNBOUND_PORT to be expanded, use an unquoted heredoc delimiter. Otherwise, ensure all values are hardcoded.
Comment on lines
+208
to
+215
| local env_file="$PROJECT_DIR/.env" | ||
| local suite_api_key="$SUITE_API_KEY_ENV" | ||
| if [[ -z "$suite_api_key" ]]; then | ||
| suite_api_key="$(extract_env_value "SUITE_API_KEY" "$env_file")" | ||
| fi | ||
| if [[ -z "$suite_api_key" ]]; then | ||
| suite_api_key="$(openssl rand -hex 16)" | ||
| fi |
There was a problem hiding this comment.
suggestion: Mehrere Quellen für SUITE_API_KEY können zu Verwirrung führen.
Bitte klären oder dokumentieren Sie die Rangfolge der SUITE_API_KEY-Zuweisung, um Verwirrung zu vermeiden, wenn sich Umgebungs- und .env-Werte unterscheiden.
Suggested change
| local env_file="$PROJECT_DIR/.env" | |
| local suite_api_key="$SUITE_API_KEY_ENV" | |
| if [[ -z "$suite_api_key" ]]; then | |
| suite_api_key="$(extract_env_value "SUITE_API_KEY" "$env_file")" | |
| fi | |
| if [[ -z "$suite_api_key" ]]; then | |
| suite_api_key="$(openssl rand -hex 16)" | |
| fi | |
| # SUITE_API_KEY assignment order of precedence: | |
| # 1. If SUITE_API_KEY_ENV is set (from environment), use it. | |
| # 2. Else, if SUITE_API_KEY is set in .env file, use that. | |
| # 3. Else, generate a random value. | |
| local env_file="$PROJECT_DIR/.env" | |
| local suite_api_key="$SUITE_API_KEY_ENV" | |
| if [[ -z "$suite_api_key" ]]; then | |
| suite_api_key="$(extract_env_value "SUITE_API_KEY" "$env_file")" | |
| fi | |
| if [[ -z "$suite_api_key" ]]; then | |
| suite_api_key="$(openssl rand -hex 16)" | |
| fi |
Original comment in English
suggestion: Multiple sources for SUITE_API_KEY may cause confusion.
Please clarify or document the order of precedence for SUITE_API_KEY assignment to prevent confusion if environment and .env values differ.
Suggested change
| local env_file="$PROJECT_DIR/.env" | |
| local suite_api_key="$SUITE_API_KEY_ENV" | |
| if [[ -z "$suite_api_key" ]]; then | |
| suite_api_key="$(extract_env_value "SUITE_API_KEY" "$env_file")" | |
| fi | |
| if [[ -z "$suite_api_key" ]]; then | |
| suite_api_key="$(openssl rand -hex 16)" | |
| fi | |
| # SUITE_API_KEY assignment order of precedence: | |
| # 1. If SUITE_API_KEY_ENV is set (from environment), use it. | |
| # 2. Else, if SUITE_API_KEY is set in .env file, use that. | |
| # 3. Else, generate a random value. | |
| local env_file="$PROJECT_DIR/.env" | |
| local suite_api_key="$SUITE_API_KEY_ENV" | |
| if [[ -z "$suite_api_key" ]]; then | |
| suite_api_key="$(extract_env_value "SUITE_API_KEY" "$env_file")" | |
| fi | |
| if [[ -z "$suite_api_key" ]]; then | |
| suite_api_key="$(openssl rand -hex 16)" | |
| fi |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
…docs cleanup
Zusammenfassung von Sourcery
Verbessert das Installationsskript durch Optimierung der DNS-Resolver-Verwaltung, des Umgangs mit Umgebungsvariablen, der Port-Erkennung und der Diensthärtung.
Fehlerbehebungen:
Verbesserungen:
Original summary in English
Summary by Sourcery
Improve the installer script by enhancing DNS resolver management, environment variable handling, port detection, and service hardening.
Bug Fixes:
Enhancements: