Repository navigation
packaging: add in-tree NS8 module (packaging/ns8) - #102
Conversation
Thin wrapper around the published app image: host-based Traefik route with cluster TLS, SQLite on the calnode-data volume, generated encryption key preserved across reconfigures. CI builds the module image with the same tag scheme as docker-publish.yml and release tags pin module and app to the same version.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
Three concrete issues in the module before merge: module :latest published from main runs the edge app (the app workflow deliberately keeps app :latest release-only, and the README tells operators to install module :latest); configure-module re-pulls a hardcoded :latest app image that the org.nethserver.images label already provides, and its unhandled .check_returncode() can abort configure; and the Traefik set-route call omits the lets_encrypt_check/lets_encrypt_cleanup flags the documented helper sets.
Reviewed changes
ns8-module.yml— validate (py_compile / JSON parse / bash -n / placeholder grep) then build the module image, publishing:edge/:latestonmain,:devondev,X.Y.Z/X.Y/lateston tags.build-images.sh— stagesimageroot, substitutes__CALNODE_APP_IMAGE__in the unit withAPP_IMAGE, builds the Vue UI in a Node builder container, commits and (optionally) pushesns8-calnode.imageroot/—configure-modulewritesstate/calnode.envandconfig.json, generates/preservesCALNODE_ENCRYPTION_KEY, and registers a host-based Traefik route;get-configurationreads back host/flags; the systemd user unit runs the app rootless on127.0.0.1:${TCP_PORT}backed by thecalnode-datavolume.ui/— Vue 2.7 Status/Settings/About pages over@nethserver/ns8-ui-lib, 13 locales, and a robot suite.- Docs —
packaging/ns8/README.mdand aCLAUDE.md"Distribution packaging" section.
I confirmed the load-bearing contracts against the app and NS8: the env names (PORT, DATABASE_URL, BASE_URL, CALNODE_ENCRYPTION_KEY) match internal/config/config.go; secrets.token_hex(32) yields the 64-hex key internal/keyvault expects; get-status is core-provided for rootless modules; and org.nethserver.images makes create-module/update-module pull the app image automatically.
ℹ️ New Settings labels exist only in en
settings.host, settings.host_helper, settings.lets_encrypt and settings.http2https were added to ui/public/i18n/en/translation.json only; every other shipped locale still carries the template's settings.test_field and none of the new keys. Because main.js does not set a fallbackLocale and only loads the visitor's two-letter locale, a non-English browser renders the raw key path (settings.host) on the module's primary configuration screen. If these are meant to be filled by Weblate later that's fine — worth a line in the README so it isn't read as shipping-ready.
Technical details
# Missing Settings translations in non-English locales
## Affected sites
- `packaging/ns8/ui/public/i18n/en/translation.json:27-30` — has the four new keys
- `packaging/ns8/ui/public/i18n/{de,es,eu,it,pt,pt_BR,ta,ar,uk}/translation.json` — lack them; several still have `settings.test_field`
- `packaging/ns8/ui/src/main.js:44-50` — `setLocaleMessage(navigatorLang, …)` with no `fallbackLocale`, so missing keys resolve to the key string, not to `en`
## Required outcome
- Non-English Settings pages show translated labels, or the gap is explicitly documented as pending Weblate.
## Suggested approach (optional)
- Ship at least an English fallback (`fallbackLocale: "en"`, load both) or translate the four labels in the existing locale files; drop the leftover `test_field` keys.ℹ️ Nitpicks
ui/public/metadata.json:16—code_urlpoints athttps://github.com/calnode/ns8-calnode, but this PR (and the README/CLAUDE.md) place the module in-tree underpackaging/ns8. Point it at the Calnode repo to avoid sending users to a separate/possibly-absent repository.
opencode-go/deepseek-v4.1-flash ｜ 𝕏
| ) | ||
|
|
||
| # Make sure the app image is present before the service starts. | ||
| agent.run_helper("podman", "pull", CALNODE_IMAGE).check_returncode() |
There was a problem hiding this comment.
This pre-pull is redundant and pulls the wrong tag. The org.nethserver.images=${app_image} label already makes create-module/update-module download the exact image the module was built with, while this line hardcodes ghcr.io/calnode/calnode:latest (20configure:28) instead of the APP_IMAGE the unit was stamped with. Because .check_returncode() is unhandled, a registry or network hiccup aborts configure-module outright even though the correct image is already local.
Technical details
# Redundant, hardcoded app-image pull
## Affected sites
- `packaging/ns8/imageroot/actions/configure-module/20configure:28` — `CALNODE_IMAGE = "ghcr.io/calnode/calnode:latest"`
- `packaging/ns8/imageroot/actions/configure-module/20configure:102-103` — the pull + `check_returncode()`
- `packaging/ns8/build-images.sh:74` — sets `org.nethserver.images=${app_image}`
- `.github/workflows/ns8-module.yml:54,57,60` — `APP_IMAGE` is `:X.Y.Z` on tags and `:edge` on main, so `:latest` is never the running tag
## Required outcome
- The app image the unit runs is present before start, without a second source of truth for its tag and without making configure fail on an avoidable pull.
## Suggested approach (optional)
- Drop the pull entirely (the label covers install and update), or resolve the already-downloaded image from the core-populated agent env (e.g. `os.environ.get("CALNODE_IMAGE")`) and use `podman-pull-missing` rather than an unconditional `podman pull`.|
|
||
| # Register a host-based route. Since the "instance" attribute starts with | ||
| # MODULE_ID, the route is removed automatically with the module. | ||
| response = agent.tasks.run( |
There was a problem hiding this comment.
Calling agent.tasks.run(..., action="set-route") directly skips the lets_encrypt_check: True and lets_encrypt_cleanup: True inputs that the documented agent.set_route() helper supplies implicitly. Without lets_encrypt_cleanup, unchecking Let's Encrypt on an already-certified hostname does not purge the stale certificate on the Traefik side.
Technical details
# set-route bypasses the documented helper defaults
## Affected sites
- `packaging/ns8/imageroot/actions/configure-module/20configure:107-117` — `agent.tasks.run(action="set-route", data=…)`
## Required outcome
- Let's Encrypt toggling (including turning it off) behaves as the NS8 docs describe.
## Suggested approach (optional)
- Use `agent.set_route({...})`, or add `"lets_encrypt_check": True` and `"lets_encrypt_cleanup": True` to the `set-route` data. See NS8 TLS-certificates docs: <https://nethserver.github.io/ns8-core/modules/certificates/>.buildah commit ran before the lowercase normalization, so the build failed on ghcr.io/Calnode (uppercase owner) with 'repository name must be lowercase'. Normalize at variable prep; every use below is safe.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Since the prior pullfrog review (45b569f), one commit landed:
- Lowercased the registry namespace before the image commit —
build-images.shnormalizesREPOBASEat variable-prep time and drops the now-redundant laterepobase/reponamelowering, so thebuildah commitreference matches the lowercase tag/push refs. This mattered because the workflow passesghcr.io/${{ github.repository_owner }}, which resolves toghcr.io/Calnodewith a capital C.
The prior review's must-address findings (module :latest on main, the redundant hardcoded pull, and the set-route Let's Encrypt flags) are untouched by this commit and remain open, so this isn't an approval.
opencode-go/deepseek-v4.1-flash ｜ 𝕏
… restore docs - main pushes module :edge only; :latest stays pinned to releases, mirroring docker-publish.yml - podman stop grace 10s -> 45s (app needs 30s HTTP drain + worker bound) - configure-module warm-pulls the stamped app ref instead of :latest - README: non-NS8 data adoption is CLI-only via encryption_key
There was a problem hiding this comment.
ℹ️ No critical issues — two minor doc/grace-period notes inline.
Reviewed changes
Since the prior pullfrog review (d89da13), one commit landed (ff47709):
- Module
:latestno longer moves onmain—ns8-module.ymlnow emitsmodule_tags=edgeforrefs/heads/main, so:latestmoves only on release tags, matchingdocker-publish.yml. This resolves the earlier:latest-runs-edge finding. - App image is stamped into
configure-module—build-images.shnow substitutes__CALNODE_APP_IMAGE__inimageroot/actions/configure-module/20configureas well as the systemd unit, andCALNODE_IMAGEis that placeholder rather than a hardcoded:latest. - Longer stop grace — the systemd unit's
ExecStopnow usespodman stop … -t 45(was-t 10) to let in-flight requests drain. - Restore/adoption docs —
README.mddocuments importing a non-NS8 SQLite database and passingencryption_keytoconfigure-module.
opencode-go/deepseek-v4.1-flash ｜ 𝕏
| builds the module image on every PR (no push) and publishes on branch and | ||
| tag pushes, mirroring the app tags in `docker-publish.yml`: | ||
|
|
||
| - push to `main` pushes module `:edge` and `:latest`, running app `:edge` |
There was a problem hiding this comment.
This bullet now contradicts the workflow. ff47709 changed main to publish :edge only, and the workflow's own header comment says :latest stays pinned to releases — but the Development section still advertises a moving :latest on main. Please drop and :latest here so the docs match ns8-module.yml.
| # 45s grace: the app allows up to 30s HTTP drain plus worker shutdown, so the | ||
| # default 10s would SIGKILL updates mid-request. Data stays safe either way | ||
| # (jobs are crash-recoverable), this is about clean connection draining. | ||
| ExecStop=/usr/bin/podman stop --ignore --cidfile %t/calnode.ctr-id -t 45 |
There was a problem hiding this comment.
The rationale above undercounts the app's shutdown. cmd/calnode/main.go drains in-flight HTTP for 30s and then waits up to another 60s for the background worker, so a slow worker stop can need ~90s; -t 45 still SIGKILLs it. Since the point of the change is clean draining, either raise the value or correct the comment (data stays safe either way).
Technical details
# ExecStop grace can truncate the app's own drain budget
## Affected sites
- `packaging/ns8/imageroot/systemd/user/calnode.service:40` — `podman stop … -t 45`
- `cmd/calnode/main.go:132` — `httpServer.Shutdown` bounded at 30s
- `cmd/calnode/main.go:144-148` — worker-drain select waits up to an additional 60s after HTTP shutdown
## Required outcome
- The unit's stop grace should not truncate the app's graceful drain, or the comment should state the real bound and accept the SIGKILL.
## Open questions for the human
- Is a clean worker shutdown wanted here, or is only request draining in scope? If the latter, keep 45s and just fix the comment.
1 participant
Thin NS8 wrapper around the published app image, kept in-tree per single-repo decision so CI ships both assets from each release.
What it does:
Layout: packaging/ns8/{imageroot,ui,tests,build-images.sh,README.md} plus .github/workflows/ns8-module.yml (validate on PRs, publish on branch/tag pushes). One channel per directory; future channels get siblings.
Untested, no NS8 node reachable: install, configure, HTTP route, update, backup/restore, robot suite, UI build, and the new workflow itself. Marked as such in packaging/ns8/README.md.