Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements the first step of a DDNS switch by migrating from router-managed DDNS to a host-based solution using ddclient with Dynu as the provider. The change introduces a staged rollout approach using a test hostname before migrating production services.
Changes:
- Added DDNS configuration (Dynu password) to encrypted secrets files
- Enhanced all sops scripts with consistent
--helpflag handling and improved error output redirection - Configured ddclient service on beast host with Dynu integration for test hostname
- Added nginx virtual host for
jf-test.ihar.devto test the DDNS setup - Documented the DDNS rollout process and validation steps
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| secrets/beast.yaml | Added encrypted Dynu password for DDNS authentication |
| secrets/_template.yaml | Added DDNS password template placeholder |
| scripts/sops-update.sh | Added help flag handling and error output redirection |
| scripts/sops-edit.sh | Added help flag handling and error output redirection |
| scripts/sops-copy.sh | Refactored argument parsing with help flag support |
| scripts/sops-cat.sh | Added help flag handling and error output redirection |
| scripts/sops-bootstrap.sh | Added help flag handling, validation, and error output redirection |
| nixos/beast/default.nix | Configured ddclient service with Dynu provider and test virtual host |
| nixos/beast/README.md | Documented DDNS test rollout process and validation steps |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
nixos/beast/README.md
Outdated
| 1. Create a Dynu hostname (example: `myhome-test.dynu.net`). | ||
| 1. At your registrar DNS, create: | ||
| - `jf-test.ihar.dev CNAME myhome-test.dynu.net` | ||
| 1. In `nixos/beast/default.nix`, set: | ||
| - `dynuTestHostname = "myhome-test.dynu.net";` | ||
| - `dynuUsername = "<your-dynu-username>";` |
There was a problem hiding this comment.
The documentation uses placeholder examples that conflict with the actual values defined in the code. The example hostname myhome-test.dynu.net and username <your-dynu-username> should reference the actual configured values (ihrachyshka-jf.freeddns.org and ihrachyshka) or clarify that these are examples to be customized.
| exit 0 | ||
| ;; | ||
| --user) | ||
| if [[ $# -lt 2 || -z "${2:-}" ]]; then |
There was a problem hiding this comment.
The condition $# -lt 2 is redundant when combined with -z \"${2:-}\". If $# is less than 2, then $2 will be unset and the second condition will be true. The check can be simplified to just -z \"${2:-}\".
| if [[ $# -lt 2 || -z "${2:-}" ]]; then | |
| if [[ -z "${2:-}" ]]; then |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 1. In `nixos/beast/default.nix`, set: | ||
| - `dynuHostname = "ihrachyshka-home.freeddns.org";` | ||
| - `dynuUsername = "ihrachyshka";` | ||
| 1. Add `ddns.dynu.password` to `secrets/beast.yaml` via `sops`. |
There was a problem hiding this comment.
The README refers to ddns.dynu.password, but the Nix config reads the secret from the SOPS key ddns/dynu/password (slash-separated path) via key = \"ddns/dynu/password\". To avoid confusion during rollout, align the documentation with the configured key path (or explicitly mention both notations and which tool expects which).
| 1. Add `ddns.dynu.password` to `secrets/beast.yaml` via `sops`. | |
| 1. Add SOPS key `ddns/dynu/password` (YAML path `ddns.dynu.password`) to `secrets/beast.yaml` via `sops`. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
scripts/sops-update.sh:1
- The logical operator should be
&&instead of||. The current condition returns 1 (failure) if either substring is missing, but the mock should only fail when both are missing. This causes the test to incorrectly validate the yq expression.
#!/usr/bin/env bash
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| local src_host="" | ||
| local dst_host="" | ||
| local key_path="" |
There was a problem hiding this comment.
These variable declarations are redundant since they're immediately assigned empty strings and then conditionally populated in the loop. Remove the explicit empty string assignments and rely on bash's default unset variable behavior in the conditional checks.
| local src_host="" | |
| local dst_host="" | |
| local key_path="" | |
| local src_host | |
| local dst_host | |
| local key_path |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
1 participant
No description provided.