PR-7/8/11: Installer wizard, workspace provisioning, compose.release generator (Wave 3) - #46
Open
jmservera wants to merge 12 commits into
Open
PR-7/8/11: Installer wizard, workspace provisioning, compose.release generator (Wave 3)#46jmservera wants to merge 12 commits into
jmservera wants to merge 12 commits into
Conversation
…nerator; integrate into install.sh
Contributor
There was a problem hiding this comment.
Pull request overview
Adds Wave 3 “Phase 3” installer artifacts: an interactive wizard to generate .env, a workspace provisioning/migration helper, and a compose.release.yaml generator that guards against unpinned images. Integrates the wizard + provisioning helper into install.sh local-mode flow and updates .env.template to include new installer-facing keys.
Changes:
- Add
scripts/installer-wizard.shto guide.envcreation and workspace setup. - Add
scripts/provision-workspace.shto ensure/migrate the workspacesrcdirectory. - Add
scripts/compose-release-gen.shto generatecompose.release.yamlwith an unpinned-image guard; wire helpers intoinstall.sh.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/provision-workspace.sh | New workspace ensure/migration helper used by the installer. |
| scripts/installer-wizard.sh | New interactive wizard to create .env and workspace directories. |
| scripts/compose-release-gen.sh | New generator for compose.release.yaml with image-tag pinning guard. |
| install.sh | Calls the wizard/provisioning helper during local installs. |
| .env.template | Adds installer-facing keys and optional override comments (currently introduces duplicate key definitions). |
Comments suppressed due to low confidence (1)
scripts/provision-workspace.sh:38
- Same as ensure_src(): migrate_workspace reads UBEROS_WORKSPACE with grep without constraining to the first match, which can produce a multi-line ws value and break path handling.
envf="${ROOT_DIR}/.env"
if [ -f "$envf" ]; then
ws="$(grep '^UBEROS_WORKSPACE=' "$envf" | cut -d= -f2-)"
else
The first pass copied compose.yaml verbatim, so the generated release compose still carried every build: context and contained no image references at all - the unpinned-tag guard passed vacuously. Replace it with scripts/gen-compose-release.sh (name per plan), which rewrites each build: block into a pinned image: ref matching the release.yml matrix, handles CRLF checkouts, and enforces the FR-B9 guard on the generated file. - add compose-release job to release.yml (generate, validate, guard, upload) - prefer source-checkout detection in install.sh so a locally generated compose.release.yaml cannot flip the tree into release mode - gitignore the generated compose.release.yaml - add scripts/validate-wave3.sh covering syntax, generation, compose config, and the unpinned-tag negative test
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 8 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (3)
install.sh:108
- run_wizard_if_needed checks the wizard script with -x, but the script is invoked via
shand may not have the executable bit set (common on Windows checkouts). This can cause the wizard to be skipped unexpectedly.
if [ -x "${WIZARD_SCRIPT}" ]; then
install.sh:145
- The provisioning helper is invoked via
sh, but the presence check uses -x. If the file exists but isn’t executable, workspace provisioning will silently fall back to the legacy path (or fail later).
if [ -x "${PROVISION_SCRIPT}" ]; then
scripts/installer-wizard.sh:62
- The wizard substitutes UBEROS_WORKSPACE using sed with an unescaped replacement. Workspace paths containing characters like
&,|, or backslashes will produce a corrupted .env. Use the same awk-based key replacement approach used for UBEROS_PORT and UBEROS_VERSION.
sed "s|^UBEROS_WORKSPACE=\$|UBEROS_WORKSPACE=${ws}|" "$envf" > "$envf.tmp" && mv "$envf.tmp" "$envf"
# Use portable sed replacement: write to temp file then move
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 8 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (4)
install.sh:30
- COMPOSE_GEN_SCRIPT points to scripts/compose-release-gen.sh, but that file doesn’t exist (the repo has scripts/gen-compose-release.sh) and the variable is never used. Keeping a wrong/unused path here is misleading and makes future changes error-prone.
# Paths to helper scripts (Phase 3 additions)
WIZARD_SCRIPT="${ROOT_DIR}/scripts/installer-wizard.sh"
PROVISION_SCRIPT="${ROOT_DIR}/scripts/provision-workspace.sh"
COMPOSE_GEN_SCRIPT="${ROOT_DIR}/scripts/compose-release-gen.sh"
install.sh:112
- run_wizard_if_needed() gates on
-xfor installer-wizard.sh, but the script is invoked viash ...and doesn’t need the executable bit. Using-xcan prevent the wizard from running in environments where Git didn’t preserve modes (e.g., some Windows checkouts).
if [ -x "${WIZARD_SCRIPT}" ]; then
# If a user explicitly set NON_INTERACTIVE, skip the wizard
if [ -t 0 ] && [ -z "${NON_INTERACTIVE:-}" ]; then
sh "${WIZARD_SCRIPT}"
else
scripts/installer-wizard.sh:64
- The workspace substitution uses
sedwith an unescaped replacement string. If the user’s workspace path contains characters like&or backslashes, sed will rewrite it incorrectly. Since you’re already using awk for the other keys, use awk for UBEROS_WORKSPACE too.
cp "$tmpl" "$envf"
sed "s|^UBEROS_WORKSPACE=\$|UBEROS_WORKSPACE=${ws}|" "$envf" > "$envf.tmp" && mv "$envf.tmp" "$envf"
# Use portable sed replacement: write to temp file then move
awk -v p="${port}" 'BEGIN{FS=OFS="="} /^UBEROS_PORT=/{ $2=p } { print }' "$envf" > "$envf.tmp" && mv "$envf.tmp" "$envf"
awk -v v="${version}" 'BEGIN{FS=OFS="="} /^UBEROS_VERSION=/{ $2=v } { print }' "$envf" > "$envf.tmp" && mv "$envf.tmp" "$envf"
.env.template:6
- The optional override comment references
UBEROS_PROXY_PORT, but the actual variable used throughout the repo (and defined just above) isUBEROS_PORT. This looks like a stale name and could confuse users editing .env.
# Optional overrides
# UBEROS_HOST=
# UBEROS_PROXY_PORT=8080
…nstall.sh The wizard and workspace provisioning were split into scripts/installer-wizard.sh and scripts/provision-workspace.sh, which duplicated installer state, missed most of the Theme E/F acceptance criteria, and were gated on the executable bit even though they were invoked via `sh`. Both scripts are removed and their behaviour is implemented in install.sh, where the answers, .env, and workspace are already owned. install.sh now provides: * --non-interactive/-y and --config <file> for silent installs; the config file is parsed against a key allowlist instead of being sourced, so it can never execute arbitrary commands (FR-E2) * validation with actionable errors for port range, absolute/outside-checkout workspace paths, writable parents, and known GPU overlays (FR-E3, FR-E4) * .env writes via awk instead of sed, so paths containing | & / cannot corrupt the substitution, and keys are replaced in place rather than appended, so no duplicate keys appear (FR-E5) * --workspace-repo seeding of an empty workspace and copy-then-verify migration of a repo-local ./workspace that never deletes the source (FR-F2, FR-F3, FR-F4) * --dry-run to resolve, validate, and provision without invoking Docker Also fixed: * .env.template: drop the duplicated UBEROS_WORKSPACE/UBEROS_PORT/UBEROS_VERSION block and the UBEROS_PROXY_PORT key, which is referenced nowhere in the repo * gen-compose-release.sh: the unpinned-tag guard treated any ":" as a tag, so localhost:5000/org/image passed; it now inspects only the last path segment * validate-wave3.sh: use an explicit mktemp template (bare mktemp fails on BSD), run installer flows in a disposable sandbox so a developer .env is never touched, and cover the CLI, validation, config-file, migration, and guard paths instead of only the happy path
…servera/UbeROS into release/PR-03-wave3-installer
…orts out of the repo Self-review of the Wave 3 diff surfaced two defects: * Re-running the installer overwrote UBEROS_PORT and UBEROS_WORKSPACE with the built-in defaults, silently discarding values the user had edited by hand. The wizard now seeds its defaults from the existing .env first, so only explicit CLI/config answers can change them. Covered by a re-run regression test. * A failed migration verification wrote .uberos-migrate-missing into the source checkout, dirtying the repo the installer is supposed to keep clean. The report is now a temp file and the missing paths are printed to stderr instead.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
scripts/gen-compose-release.sh:66
- The unpinned-tag guard currently only treats ":latest" (and missing tags) as floating. Since the release workflow also publishes a moving ":beta" tag for pre-releases, the guard should reject ":beta" too; otherwise a compose file could reference a floating tag and still pass the FR-B9 check.
last = probe
sub(/.*\//, "", last)
if (probe ~ /:latest$/ || last !~ /:/) {
printf " unpinned image reference on line %d: %s\n", NR, ref > "/dev/stderr"
The repository was already 100% LF in the index, but nothing normalized on check-in, so the Windows working tree had drifted to CRLF on 127 files. That churn made every git status noisy, blocked merges with "local changes would be overwritten", and silently shipped CRLF into container images through the COPY . . in the frontend and gzweb-client builds. The previous per-extension allowlist could not keep up: .gzlaunch, .sdf, .yaml, .js, .svelte, .json and .css all reach Linux images with no rule covering them. Replace it with a single "* text=auto eol=lf" so LF is the default and binaries are still detected and left alone. Add .editorconfig and files.eol in .vscode/settings.json so editors write LF back instead of reintroducing the drift.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 8 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
install.sh:507
UBEROS_MIGRATEvalues coming from--configare not validated up front. If the config contains a typo (e.g.UBEROS_MIGRATE=yees) and there is no legacy./workspace/srcto migrate, the installer will proceed without surfacing the invalid answer, contradicting the goal of validating all provided answers early.
if [ -n "${CONFIG_FILE}" ]; then
load_config "${CONFIG_FILE}"
fi
# Validate anything supplied up front so mistakes surface before any prompting
# or Docker work happens.
if [ -n "${OPT_PORT}" ]; then
validate_port "${OPT_PORT}" || exit 1
fi
if [ -n "${OPT_GPU}" ]; then
validate_gpu "${OPT_GPU}" || exit 1
fi
…review Second round of review feedback on the Wave 3 installer and release-compose work. gen-compose-release.sh read UBEROS_IMAGE_NAMESPACE while release.yml exported IMAGE_NAMESPACE, so changing the workflow's registry would have silently kept generating compose.release.yaml against the old one. The workflow now passes its own value through to the generator. The unpinned-tag guard only rejected :latest, but release.yml republishes a moving :beta on every pre-release; a bundle pinned to it would change under the user's feet. The guard now rejects :beta too, while still accepting immutable pre-release version tags such as :0.4.0-beta. install.sh seeded OPT_WORKSPACE from an exported UBEROS_WORKSPACE, and load_config only writes into an empty OPT_*, so --config could never override the environment. The environment is now kept in ENV_WORKSPACE and applied as a wizard default, giving a documented precedence of command line > --config > environment > existing .env > built-in default. The wizard claimed command-line answers were never re-asked but prompted for them anyway, using them as defaults and overwriting them with whatever was typed. It now prompts only for the answers nobody supplied, and validates the final set once on every path. The migration copy used a plain tar pipeline, whose exit status is the extractor's; a failure while reading the legacy workspace could be reported as a successful migration. The producer's status is now recorded and checked. Adds five regression tests (29 -> 32): the three precedence orderings, the moving :beta rejection, and the immutable :0.4.0-beta acceptance. The harness also clears UBEROS_WORKSPACE so results no longer depend on the developer's environment.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 10 changed files in this pull request and generated no new comments.
Files not reviewed (1)
- services/gazebo/client/package-lock.json: Generated file
Comments suppressed due to low confidence (1)
services/gazebo/client/package-lock.json:27
- This lockfile now hard-codes tarball URLs to
https://ms-feed-*.pkgs.visualstudio.com/...(e.g. @babel/runtime). That host is environment-specific and can makenpm cifail for contributors/CI that don’t have access, and it’s inconsistent with the repo’s other lockfiles (which resolve viaregistry.npmjs.org). Regenerate this lockfile using the public npm registry (or whatever registry is intended for the project) so installs are reproducible for everyone (e.g. runnpm install --package-lock-only --registry=https://registry.npmjs.org/fromservices/gazebo/client/and commit the updated lock).
The lockfile was last regenerated behind a corporate npm proxy, so all 87 entries pointed at internal `*.pkgs.visualstudio.com` feed URLs with legacy sha1 integrity. Those hosts are unreachable from GitHub-hosted runners, and `npm ci` reads `resolved` directly, so the gazebo client image build could fail in CI even though it built fine locally. Every entry now resolves from registry.npmjs.org with the upstream sha512 integrity. No dependency version changed: each internal-feed sha1 was checked against the public registry's shasum first, and all 87 matched, so both feeds serve byte-identical tarballs. vite stays pinned at 8.1.3 to respect the one-week package curfew. This also restores local builds behind the proxy: npm's default `replace-registry-host=npmjs` rewrites registry.npmjs.org tarball URLs to whatever registry is configured, which the hard-coded feed URLs prevented.
migrate_legacy_workspace() only checked that the source ./workspace/src had content, not the target. Since the copy is a tar extraction, running the installer against an external workspace that already held packages replaced same-path files with no warning and no way to tell the user what was lost. Two paths reached it: the interactive prompt defaults to "y", so pressing Enter on a re-run was enough, and --migrate / UBEROS_MIGRATE=yes skipped the prompt entirely. The post-copy verification could not catch this either, as it only checks that source files arrived, never that target files survived. Migration now refuses a target that already has content, the same rule seed_workspace_repo applies to cloning, which makes --migrate safe by construction instead of relying on the user reading a prompt. Covered by a new check in scripts/validate-wave3.sh (33 total, all passing): it pre-populates a workspace, installs with --migrate, and asserts the existing file is untouched and demo_pkg was not merged in. Verified as a real regression test by running the same scenario against the pre-fix install.sh, which does merge demo_pkg in.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 10 changed files in this pull request and generated no new comments.
Files not reviewed (1)
- services/gazebo/client/package-lock.json: Generated file
Comments suppressed due to low confidence (2)
install.sh:233
- validate_workspace() currently rejects a workspace path when its parent directory is not writable, even if the workspace directory already exists and is writable (e.g., /opt/ws owned by the user but /opt not writable). That’s an unnecessary hard failure and can block valid existing workspaces.
if [ ! -d "${_parent}" ]; then
err "workspace parent directory '${_parent}' does not exist; create it first or choose another path"
return 1
fi
if [ ! -w "${_parent}" ]; then
install.sh:426
- If tar creation fails (left side of the pipeline) but extraction still exits 0, the code correctly detects the failure via _tar_status and dies — but it leaves whatever was extracted so far in the target workspace. Since the target is guaranteed empty before migration, it’s safer to clean it up on failure so a subsequent run can retry migration cleanly.
} | ( cd "${_target}" && tar xf - ) || {
rm -f "${_tar_status}"
die "migration copy failed; ${LEGACY_WORKSPACE}/src was left untouched"
}
Two issues Copilot raised as low-confidence on the previous commit turned out to be real, and the second one was made worse by the emptiness guard added in b33af9b. validate_workspace() required the *parent* of the workspace to be writable even when the workspace itself already existed. That rejects perfectly ordinary setups such as a user-owned /opt/uberos-workspace under a root-owned /opt, where nothing has to be created in the parent at all. The parent checks now apply only when the workspace still needs to be created; an existing path must instead be a writable directory, and a dangling symlink is reported plainly rather than failing later inside mkdir -p. migrate_legacy_workspace() died on a failed copy but left whatever tar had already extracted in the target. Combined with the new "never merge into a populated workspace" guard, the next run would see that debris, skip the migration and leave the user stranded with half their packages. Every failure path now removes the partial copy first. That rm -rf is safe by construction: it only ever runs after the guard proved the target was empty, and the helper refuses any path that is not a <workspace>/src. seed_workspace_repo() was checked for the same failure mode; git already cleans up after a failed clone into a pre-existing directory, so it needs no change. Wave 3 harness: 33 -> 37 checks. Negative controls against the pre-fix installer confirm both new tests are real regression tests - the old code rejected the read-only-parent workspace and left demo_pkg behind after a failed copy. The permission-dependent checks are skipped when running as root, where the mode bits would prove nothing.
Owner
Author
|
Both comments suppressed as low confidence on the last review were real, so
I also checked Harness is at 37 checks. Negative controls against the pre-fix installer confirm both new tests are genuine regression tests — the old code rejected the read-only-parent workspace and left |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 10 changed files in this pull request and generated no new comments.
Files not reviewed (1)
- services/gazebo/client/package-lock.json: Generated file
Comments suppressed due to low confidence (2)
install.sh:605
- The “validate anything supplied up front” block validates --port and --gpu but not --workspace. If a user supplies an invalid workspace (e.g., relative path or inside the checkout) alongside missing port/GPU, the installer can still prompt for the other values and only then fail, which contradicts the intent of this block and makes failures noisier than needed.
# Validate anything supplied up front so mistakes surface before any prompting
# or Docker work happens.
if [ -n "${OPT_PORT}" ]; then
validate_port "${OPT_PORT}" || exit 1
fi
if [ -n "${OPT_GPU}" ]; then
validate_gpu "${OPT_GPU}" || exit 1
fi
services/gazebo/client/package.json:21
- This change broadens the Vite version range from ^8.1.5 to ^8.1, and the updated lockfile resolves Vite to 8.1.3 (a downgrade). If the downgrade isn’t intentional, it would be safer to keep the previous minimum patch (or otherwise specify the desired patch) and re-generate the lockfile so builds don’t silently regress.
"vite": "^8.1"
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
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.
Wave 3 — Installer UX, workspace provisioning, release compose
Implements Phase 3 of the PRD 004 release-packaging plan: PR-7 (installer wizard), PR-8 (workspace provisioning + migration), PR-11 (
compose.release.yamlgenerator + unpinned-tag guard).What changed
install.sh— guided setup, silent installs, validation (FR-E1…FR-E5)--non-interactive/-yand--config <file>for silent installs. The config file is parsed line by line against a key allowlist rather than sourced, so an answers file can never execute arbitrary commands..envis generated from.env.templateon first run and never clobbered. Values are written withawk, notsed, so paths containing|,&, or/cannot corrupt the substitution, and keys are replaced in place so no duplicates accumulate.--dry-runresolves, validates, and provisions without invoking Docker.install.sh— workspace provisioning (FR-F2, FR-F3, FR-F4)<workspace>/srcon the host before compose bind-mounts it, so Docker never creates it as root.--workspace-repo <git>seeds an empty workspace from a repository../workspace/src: every source file is confirmed present in the target before success is reported, and the source is never deleted automatically.--migrate/--no-migrateforce the decision non-interactively.scripts/gen-compose-release.sh— release compose (FR-B4, FR-B9)build:block incompose.yamlintoimage: ghcr.io/jmservera/uberos/<service>:<version>, matching thepublish-imagesbuild matrix inrelease.yml.:latest, fails the build and the offending line and reference are printed. Only the last path segment is inspected, so a registry host:port is not mistaken for a tag.--check <file>runs the guard standalone; thecompose-releaseCI job generates the file, validates it withdocker compose config, re-runs the guard, and uploads it as a release artifact.scripts/validate-wave3.sh— validation harness covering the Wave 3 success criteria. Installer flows run inside a disposable sandbox, so a developer's.envand workspace are never touched.Review feedback
All 11 review comments are resolved. The most significant one: the wizard and provisioning had been split into
scripts/installer-wizard.shandscripts/provision-workspace.sh, which duplicated installer state, were gated on the executable bit despite being invoked viash(so they would silently never run on a checkout without mode bits), and missed most of the Theme E/F acceptance criteria. Both scripts are removed and their behaviour is implemented ininstall.sh, which already owns the answers, the.env, and the workspace.Also fixed from review: duplicated keys accidentally prepended to
.env.template, the unusedUBEROS_PROXY_PORTkey, the registry-port false negative in the guard, a non-portable baremktemp, and a staleCOMPOSE_GEN_SCRIPTpath.Validation
compose.release.yamlgenerated forv0.4.0pins all 8 UbeROS service images and leaves the upstreamdiscovery-serverimage untouched:Notes
compose.release.yamlis gitignored — it is a build artifact produced at release time, not a committed file. Mode auto-detection tests for a source checkout first so a locally generated copy cannot flip a dev tree into release mode.