mirror of
https://github.com/maziggy/bambuddy.git
synced 2026-10-09 15:35:39 +02:00
fix(updates): install the discovered release tag, not hardcoded origin/main
The in-app updater ran `git fetch origin main && git reset --hard origin/main` regardless of which version the GitHub releases API reported as latest. So whenever the latest release lived on a branch other than main — e.g. during a beta cycle when 0.2.4b1 sits on its own branch and main still points at the previous stable — clicking Apply Update appeared to succeed but the user actually stayed pinned to old main HEAD. Fix: extract `_discover_target_release(db)` mirroring the same release-API + include_beta_updates selection the GUI's update-check already uses, pass the resolved tag (e.g. `v0.2.4b1`) into `_perform_update(target_ref)`, and run `git fetch --prune --tags origin && git reset --hard <target_ref>`. The fetch now pulls --tags so a tag ref is locally resolvable; the reset takes the caller's ref instead of a hardcoded branch. apply_update now returns a clear error if no release resolves, instead of silently kicking off an update that can't land.
This commit is contained in:
@@ -49,6 +49,7 @@ All notable changes to Bambuddy will be documented in this file.
|
||||
- **i18n: full key parity across all 8 locales** — `en` is the reference; every other locale (`de`, `fr`, `it`, `ja`, `pt-BR`, `zh-CN`, `zh-TW`) is checked identically and any drift fails CI. Until now, the parity script at `frontend/scripts/check-i18n-parity.mjs` only enforced parity for `de` / `zh-CN` / `zh-TW` and demoted `fr` / `it` / `ja` / `pt-BR` to an "informational" tier — drift was reported but never gated. Result: 78 missing keys in fr and it, 66 in pt-BR, 54 in ja, accumulated across every release that added new en strings. Backfilled real translations (not English fallbacks) for every gap: `login.resetPassword.*` (12 keys, fr/it/ja), `printers.firmwareModal.*` extension (7 keys × 4 locales) from the firmware modal redesign, the full `settings.spoolbuddy.*` device-control admin block (~40 keys) for unregister / reboot / shutdown / update / restart confirms, the kiosk-side `spoolbuddy.settings.*` block (13 keys × 4) for backend & auth + diagnostics, and the new `virtualPrinter.archiveNameSource.*` block from this release ([#1152](https://github.com/maziggy/bambuddy/issues/1152)). The parity script itself dropped the two-tier `STRICT` / `info` machinery — every non-en locale is now treated equally — so any future feature that adds en strings without translating them everywhere fails CI uniformly. All 8 locales sit at 4492 leaves.
|
||||
|
||||
### Fixed
|
||||
- **In-app upgrade was hardcoded to `origin/main` and silently no-op'd whenever the latest release wasn't on main** — `_perform_update` ran `git fetch origin main && git reset --hard origin/main` verbatim, regardless of which version GitHub's releases API reported as latest. So during any beta release cycle (when `0.2.4b1` lives on its own branch and `main` still points at the previous stable), users on the prior stable who clicked *Apply Update* saw the GUI report success but actually stayed pinned to the old `main` HEAD. The pre-existing pip-cwd and SSH-origin-clobber bugs in this same code path made it worse, but the underlying limitation was that the updater literally couldn't reach a non-main release. Fix: extract `_discover_target_release(db)` (mirrors the same release-API + `include_beta_updates` selection logic the GUI's update-check route already uses), pass the resolved tag (e.g. `v0.2.4b1`) into `_perform_update(target_ref)`, and `git fetch --prune --tags origin && git reset --hard <tag>`. The fetch step now pulls `--tags` so the tag ref is locally resolvable; the reset takes whatever ref the caller resolved instead of a hardcoded branch. Also makes `apply_update` return a clear error if no release matches the user's channel rather than silently kicking off an update that can't land. Three new regression tests in `test_updates_api.py` cover (1) `_perform_update` resets to the caller-supplied ref and fetches tags, (2) `apply_update` plumbs the discovered tag through to `_perform_update`, (3) `apply_update` errors out cleanly when discovery returns no candidate.
|
||||
- **In-app upgrade clobbered SSH `origin` on developer checkouts** — The in-app *Apply Update* path unconditionally ran `git remote set-url origin https://github.com/maziggy/bambuddy.git` before fetching, on the assumption that systemd service users wouldn't have SSH keys configured. That assumption holds for production native installs, but anyone testing the upgrade flow against their own development checkout (where `origin` is legitimately `git@github.com:maziggy/bambuddy.git` and authentication is via SSH keys) had their SSH origin silently rewritten to HTTPS — so the very next `git push` prompted for HTTPS credentials they didn't have configured and bounced. Fix: the updater now reads the current `origin` first via `git remote get-url`, parses the URL into an `(owner, repo)` pair (handling all four canonical forms — `git@github.com:owner/repo[.git]` and `https://github.com/owner/repo[.git]`), and only rewrites if it doesn't already resolve to `maziggy/bambuddy`. Native installs with no remote set, or origins pointing at a fork / wrong repo, still get reset to the canonical HTTPS URL. Three regression tests in `test_updates_api.py` cover the parser, the SSH-preservation case, and the fork-rewrite case so a future refactor can't regress either side of the contract.
|
||||
- **Native-install in-app upgrade silently skipped `pip install` and the new dependencies never landed** — On a native install (where systemd sets `DATA_DIR=$INSTALL_PATH/data`), the in-app *Apply Update* button shipped the new code via `git reset --hard origin/main` correctly but then logged `ERROR: Could not open requirements file: [Errno 2] No such file or directory: 'requirements.txt'` and continued without installing the new deps. `pip install -r requirements.txt` was running with `cwd=settings.base_dir`, which on a native install resolves to the data dir (e.g. `/opt/bambuddy/data`), not the source-code dir (`/opt/bambuddy`); pip doesn't walk up looking for the requirements file the way `git` walks up looking for `.git`, so the file wasn't found, the install was effectively skipped, and the user ended up with new code but stale dependencies — which surfaces as cryptic import / runtime errors on the next restart. Same bug affected the optional `npm install` / `npm run build` step (it tested `frontend_dir = base_dir / "frontend"`, which doesn't exist on native installs, and silently fell through to the pre-built static files). Fix: introduce `settings.app_dir` alongside `settings.base_dir` pointing at the source-tree root, and run `pip install` and the npm steps with `cwd=settings.app_dir`. Git operations keep using `base_dir` since they already worked (git walks up to find `.git`). Docker users were unaffected — Docker doesn't use the in-app updater (image pull replaces it). Regression test in `test_updates_api.py` mocks every subprocess invocation in `_perform_update`, captures their cwd, and asserts the pip step runs in `app_dir` and that `requirements.txt` actually exists there, so a future refactor that re-introduces `cwd=base_dir` for the pip step fails CI before another user trips over it.
|
||||
- **Postgres restore from a SQLite Local Backup aborted with `cannot drop table printers`** — Settings → Backup → Restore on a Bambuddy running against external Postgres failed with `asyncpg.exceptions.DependentObjectsStillExistError: cannot drop table printers because other objects depend on it` whenever the live database carried orphan tables from removed features — for example legacy `spoolman_slot_assignments` / `spoolman_k_profile` from an earlier Spoolman integration that has since been removed from the ORM but whose tables and `*_printer_id_fkey` constraints still sat in the live schema, pointing at `printers`. The restore path (`_import_sqlite_to_postgres` in `settings.py`) called `metadata.drop_all`, which only enumerates tables defined by SQLAlchemy ORM models and emits plain `DROP TABLE` (no `CASCADE`); Postgres correctly refused to drop `printers` while external constraints still referenced it, the entire restore aborted before any rows landed, and the user was left without a working DB. The drop phase now executes `DROP TABLE … CASCADE` on every table in the `public` schema (via a `pg_tables`-iterating PL/pgSQL `DO` block, after FKs have been stripped from the ORM metadata) before `metadata.create_all` rebuilds the schema. CASCADE is the right tool for a destructive restore — the user has explicitly chosen to wipe the DB and replace it from backup, so taking out orphan tables alongside ORM tables is correct behaviour, not surprise data loss. SQLite restores are unaffected (they go through a separate path). Discovered while attempting to restore a 0.2.4b1 backup onto a Postgres instance that had been upgraded across the Spoolman integration rewrite. Two regression tests in `test_postgres_restore_drop_cascade.py` mock the Postgres engine, run `_import_sqlite_to_postgres` against a tiny SQLite source, and assert (1) the captured SQL stream contains a CASCADE-aware iteration over `pg_tables` (so a regression to `metadata.drop_all` fails CI loudly, before another user trips on it) and (2) the CASCADE drop is scoped to `schemaname = 'public'` so a shared Postgres instance holding non-Bambuddy data in other schemas isn't taken out by a restore. All 44 existing settings-API tests still pass unchanged.
|
||||
|
||||
@@ -383,8 +383,61 @@ async def check_for_updates(
|
||||
}
|
||||
|
||||
|
||||
async def _perform_update():
|
||||
"""Perform the actual update using git fetch and reset."""
|
||||
async def _discover_target_release(db: AsyncSession) -> str | None:
|
||||
"""Look up the tag we should install from GitHub releases.
|
||||
|
||||
Same selection logic the GUI's update-check uses: respect
|
||||
`include_beta_updates`, skip prereleases when the user opted out, take
|
||||
the first matching release. Returns the raw tag name (e.g. `v0.2.4b1`)
|
||||
so the git ref is unambiguous, or None if there's no release to install.
|
||||
|
||||
The previous in-app updater path was hardcoded to `git fetch origin main
|
||||
&& git reset --hard origin/main`, which silently no-ops whenever main
|
||||
isn't where the latest release lives — e.g. during a beta release cycle
|
||||
where the next stable hasn't been merged to main yet. Anchoring to the
|
||||
release tag instead lets the GUI install whatever GitHub says is latest.
|
||||
"""
|
||||
result = await db.execute(select(Settings).where(Settings.key == "include_beta_updates"))
|
||||
beta_setting = result.scalar_one_or_none()
|
||||
include_beta = beta_setting and beta_setting.value.lower() == "true"
|
||||
|
||||
try:
|
||||
async with httpx.AsyncClient() as client:
|
||||
response = await client.get(
|
||||
f"https://api.github.com/repos/{GITHUB_REPO}/releases?per_page=20",
|
||||
headers={"Accept": "application/vnd.github.v3+json"},
|
||||
timeout=10.0,
|
||||
)
|
||||
response.raise_for_status()
|
||||
releases = response.json()
|
||||
except (httpx.HTTPError, ValueError) as exc:
|
||||
logger.error("Could not fetch GitHub releases for update target: %s", exc)
|
||||
return None
|
||||
|
||||
for release in releases:
|
||||
tag = release.get("tag_name", "")
|
||||
if not tag:
|
||||
continue
|
||||
if include_beta:
|
||||
return tag
|
||||
# Skip prereleases (parsed from version, not GitHub flag — GitHub's
|
||||
# is_prerelease flag isn't always set on dailies).
|
||||
parsed = parse_version(tag)
|
||||
if parsed[4] == 0:
|
||||
return tag
|
||||
return None
|
||||
|
||||
|
||||
async def _perform_update(target_ref: str):
|
||||
"""Perform the actual update using git fetch and reset.
|
||||
|
||||
`target_ref` is whatever git ref the caller wants to land on — typically
|
||||
a release tag like `v0.2.4b1` resolved by `_discover_target_release`,
|
||||
but accepts any ref `git reset --hard` understands (`origin/main`, a
|
||||
branch, a sha). Tag-based refs are the production path because they pin
|
||||
the install to a specific release artifact instead of whatever happens
|
||||
to be on a moving branch.
|
||||
"""
|
||||
global _update_status
|
||||
|
||||
try:
|
||||
@@ -447,13 +500,18 @@ async def _perform_update():
|
||||
"error": None,
|
||||
}
|
||||
|
||||
# Fetch from origin
|
||||
# Fetch branches AND tags from origin so any ref the caller passes
|
||||
# (release tag like `v0.2.4b1`, a branch like `main`, or a sha) is
|
||||
# locally resolvable for the reset below. `--tags` is required —
|
||||
# plain `git fetch origin` doesn't bring tags by default, so a
|
||||
# release tag would not be resolvable.
|
||||
process = await asyncio.create_subprocess_exec(
|
||||
git_path,
|
||||
*git_config,
|
||||
"fetch",
|
||||
"--prune",
|
||||
"--tags",
|
||||
"origin",
|
||||
"main",
|
||||
cwd=str(base_dir),
|
||||
stdout=asyncio.subprocess.PIPE,
|
||||
stderr=asyncio.subprocess.PIPE,
|
||||
@@ -478,13 +536,18 @@ async def _perform_update():
|
||||
"error": None,
|
||||
}
|
||||
|
||||
# Hard reset to origin/main (clean update, no merge conflicts)
|
||||
# Hard reset to the target ref (clean update, no merge conflicts).
|
||||
# `target_ref` is typically a release tag like `v0.2.4b1` resolved
|
||||
# from the GitHub releases API by `_discover_target_release`. The
|
||||
# local branch name doesn't change — only HEAD moves. Falling back
|
||||
# to `origin/main` here was the source of the "in-app updater can't
|
||||
# reach beta releases" bug.
|
||||
process = await asyncio.create_subprocess_exec(
|
||||
git_path,
|
||||
*git_config,
|
||||
"reset",
|
||||
"--hard",
|
||||
"origin/main",
|
||||
target_ref,
|
||||
cwd=str(base_dir),
|
||||
stdout=asyncio.subprocess.PIPE,
|
||||
stderr=asyncio.subprocess.PIPE,
|
||||
@@ -593,6 +656,7 @@ async def _perform_update():
|
||||
@router.post("/apply")
|
||||
async def apply_update(
|
||||
background_tasks: BackgroundTasks,
|
||||
db: AsyncSession = Depends(get_db),
|
||||
_: User | None = RequirePermissionIfAuthEnabled(Permission.SETTINGS_UPDATE),
|
||||
):
|
||||
"""Apply available update (git pull + rebuild)."""
|
||||
@@ -617,8 +681,22 @@ async def apply_update(
|
||||
),
|
||||
}
|
||||
|
||||
# Discover which release tag to install. Resolved here (where we have
|
||||
# a DB session) and passed into the background task; the BG task can't
|
||||
# reuse this request's session since FastAPI closes it on response.
|
||||
target_ref = await _discover_target_release(db)
|
||||
if target_ref is None:
|
||||
return {
|
||||
"success": False,
|
||||
"message": (
|
||||
"Could not determine a release to install. Either GitHub is "
|
||||
"unreachable or no release matches your update channel "
|
||||
"(check the include_beta_updates setting)."
|
||||
),
|
||||
}
|
||||
|
||||
# Start update in background
|
||||
background_tasks.add_task(_perform_update)
|
||||
background_tasks.add_task(_perform_update, target_ref)
|
||||
|
||||
_update_status = {
|
||||
"status": "downloading",
|
||||
|
||||
@@ -23,9 +23,16 @@ class TestUpdatesAPI:
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_apply_update_non_docker(self, async_client: AsyncClient):
|
||||
"""Test non-Docker path - mock _perform_update to prevent side effects."""
|
||||
"""Test non-Docker path - mock _perform_update + _discover_target_release
|
||||
to prevent side effects (network call to GitHub releases API + actual
|
||||
git/pip subprocesses)."""
|
||||
with (
|
||||
patch("backend.app.api.routes.updates._is_docker_environment", return_value=False),
|
||||
patch(
|
||||
"backend.app.api.routes.updates._discover_target_release",
|
||||
new_callable=AsyncMock,
|
||||
return_value="v9.9.9",
|
||||
),
|
||||
patch("backend.app.api.routes.updates._perform_update", new_callable=AsyncMock),
|
||||
):
|
||||
response = await async_client.post("/api/v1/updates/apply")
|
||||
@@ -118,7 +125,7 @@ class TestUpdatesAPI:
|
||||
side_effect=fake_create_subprocess_exec,
|
||||
),
|
||||
):
|
||||
await updates_module._perform_update()
|
||||
await updates_module._perform_update("v0.2.4b1")
|
||||
|
||||
# The updater MUST NOT have run `git remote set-url origin <https>`
|
||||
# because origin already pointed at the right repo over SSH.
|
||||
@@ -167,7 +174,7 @@ class TestUpdatesAPI:
|
||||
side_effect=fake_create_subprocess_exec,
|
||||
),
|
||||
):
|
||||
await updates_module._perform_update()
|
||||
await updates_module._perform_update("v0.2.4b1")
|
||||
|
||||
set_url_calls = [c for c in calls if "set-url" in c["args"] and "origin" in c["args"]]
|
||||
assert set_url_calls, "Updater must rewrite origin when it points at a fork."
|
||||
@@ -176,6 +183,121 @@ class TestUpdatesAPI:
|
||||
f"Expected origin to be reset to canonical HTTPS URL; got: {rewritten_to}"
|
||||
)
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_perform_update_resets_to_target_ref_not_hardcoded_main(self, tmp_path):
|
||||
"""Regression for the hardcoded-`origin/main` limitation: the in-app
|
||||
updater must reset to the caller-supplied target ref (typically a
|
||||
release tag like `v0.2.4b1` discovered from the GitHub releases API)
|
||||
so beta releases that don't live on main can actually be installed.
|
||||
Pre-fix, `_perform_update` issued `git reset --hard origin/main`
|
||||
verbatim and silently no-op'd whenever the latest release wasn't on
|
||||
main — leaving a 0.2.3.x user clicking *Apply Update* stranded on
|
||||
0.2.3.x. Also asserts the fetch step uses `--tags` so a tag ref is
|
||||
actually resolvable post-fetch."""
|
||||
from backend.app.api.routes import updates as updates_module
|
||||
|
||||
app_dir = tmp_path / "app"
|
||||
data_dir = tmp_path / "app" / "data"
|
||||
app_dir.mkdir()
|
||||
data_dir.mkdir()
|
||||
(app_dir / "requirements.txt").write_text("fastapi\n")
|
||||
|
||||
calls: list[dict] = []
|
||||
|
||||
async def fake_create_subprocess_exec(*args, **kwargs):
|
||||
calls.append({"args": args, "cwd": kwargs.get("cwd")})
|
||||
proc = MagicMock()
|
||||
if "get-url" in args and "origin" in args:
|
||||
proc.communicate = AsyncMock(return_value=(b"git@github.com:maziggy/bambuddy.git\n", b""))
|
||||
else:
|
||||
proc.communicate = AsyncMock(return_value=(b"", b""))
|
||||
proc.returncode = 0
|
||||
return proc
|
||||
|
||||
with (
|
||||
patch.object(updates_module.settings, "base_dir", data_dir),
|
||||
patch.object(updates_module.settings, "app_dir", app_dir),
|
||||
patch.object(updates_module, "_find_executable", return_value="/usr/bin/git"),
|
||||
patch.object(
|
||||
updates_module.asyncio,
|
||||
"create_subprocess_exec",
|
||||
side_effect=fake_create_subprocess_exec,
|
||||
),
|
||||
):
|
||||
await updates_module._perform_update("v0.2.4b1")
|
||||
|
||||
# Reset target must be the caller-supplied ref, not "origin/main".
|
||||
reset_calls = [c for c in calls if "reset" in c["args"] and "--hard" in c["args"]]
|
||||
assert reset_calls, "git reset must be invoked"
|
||||
reset_target = reset_calls[0]["args"][-1]
|
||||
assert reset_target == "v0.2.4b1", (
|
||||
f"Expected reset target to be the caller-supplied ref 'v0.2.4b1'; "
|
||||
f"got {reset_target!r}. Regression to a hardcoded 'origin/main' "
|
||||
"would re-introduce the in-app-updater-can't-install-betas bug."
|
||||
)
|
||||
|
||||
# Fetch must include --tags so v0.2.4b1 (a tag) is locally resolvable.
|
||||
fetch_calls = [c for c in calls if "fetch" in c["args"]]
|
||||
assert fetch_calls
|
||||
assert "--tags" in fetch_calls[0]["args"], (
|
||||
"Fetch must use --tags so release-tag refs (the production path "
|
||||
"for tag-based updates) are resolvable for the subsequent reset. "
|
||||
f"Captured fetch call: {fetch_calls[0]['args']}"
|
||||
)
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_apply_update_passes_discovered_release_to_perform_update(self, async_client: AsyncClient):
|
||||
"""End-to-end glue: the route handler calls `_discover_target_release`
|
||||
to pick the tag (respecting include_beta_updates), then schedules
|
||||
`_perform_update` with that tag — not with no arg, not with main."""
|
||||
from backend.app.api.routes import updates as updates_module
|
||||
|
||||
captured_ref: list[str] = []
|
||||
|
||||
async def fake_perform_update(target_ref):
|
||||
captured_ref.append(target_ref)
|
||||
|
||||
async def fake_discover(_db):
|
||||
return "v0.2.4b1"
|
||||
|
||||
with (
|
||||
patch.object(updates_module, "_is_docker_environment", return_value=False),
|
||||
patch.object(updates_module, "_perform_update", side_effect=fake_perform_update),
|
||||
patch.object(updates_module, "_discover_target_release", side_effect=fake_discover),
|
||||
):
|
||||
response = await async_client.post("/api/v1/updates/apply")
|
||||
|
||||
assert response.json()["success"] is True
|
||||
assert captured_ref == ["v0.2.4b1"], (
|
||||
f"apply_update must pass the discovered tag to _perform_update; captured invocations: {captured_ref}"
|
||||
)
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_apply_update_returns_clear_error_when_no_release_resolves(self, async_client: AsyncClient):
|
||||
"""If GitHub is unreachable or no release matches the user's channel,
|
||||
the route returns a useful error instead of silently kicking off an
|
||||
update that can't possibly land. Avoids the previous failure mode
|
||||
where in-app update appeared to succeed but did nothing."""
|
||||
from backend.app.api.routes import updates as updates_module
|
||||
|
||||
async def fake_discover(_db):
|
||||
return None
|
||||
|
||||
# The route guards against a concurrent update via the module-global
|
||||
# `_update_status` — reset it so a previous test that left the status
|
||||
# mid-flight doesn't short-circuit this one.
|
||||
updates_module._update_status = {"status": "idle", "progress": 0, "message": "", "error": None}
|
||||
|
||||
with (
|
||||
patch.object(updates_module, "_is_docker_environment", return_value=False),
|
||||
patch.object(updates_module, "_discover_target_release", side_effect=fake_discover),
|
||||
):
|
||||
response = await async_client.post("/api/v1/updates/apply")
|
||||
|
||||
body = response.json()
|
||||
assert body["success"] is False
|
||||
assert "release" in body["message"].lower()
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_perform_update_runs_pip_in_app_dir_not_data_dir(self, tmp_path):
|
||||
"""Native install: `requirements.txt` lives at INSTALL_PATH (the source-
|
||||
@@ -220,7 +342,7 @@ class TestUpdatesAPI:
|
||||
side_effect=fake_create_subprocess_exec,
|
||||
),
|
||||
):
|
||||
await updates_module._perform_update()
|
||||
await updates_module._perform_update("v0.2.4b1")
|
||||
|
||||
# Find the pip invocation (sys.executable + "-m" + "pip" + "install").
|
||||
pip_calls = [c for c in calls if "pip" in c["args"] and "install" in c["args"]]
|
||||
|
||||
Reference in New Issue
Block a user