Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions admin/backend/api/v1/setup/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
from admin.backend.api.v1.setup.database import local_database_credentials
from admin.backend.api.v1.setup.state import setup_handoff_task
from pilot.config import BenchConfig
from pilot.config.bench import get_framework_branch_error
from pilot.internal.validators import validate_branch_name, validate_repo_url

_PASSWORD_KEYS = ("admin_password", "mariadb_password", "postgres_password")
Expand Down Expand Up @@ -36,6 +37,8 @@ def validate_configuration(data: dict) -> str | None:
return error
if error := _validate_existing_database(data):
return error
if error := get_framework_branch_error(data.get("db_type", "mariadb"), data.get("app_branch", "")):
return error
Comment on lines +40 to +41

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 P1 Missing app_branch defaults to "" and rejects valid postgres payloads

data.get("app_branch", "") turns an absent branch into "", so any postgres payload that does not carry app_branch now fails with "PostgreSQL benches need Frappe branch 'develop', not ''." — which is why test_validate_configuration_accepts_postgres_password had to add app_branch to a payload that previously passed. _update_configuration merges current flat settings, but read_flat yields app_branch == "" for a bench.toml with no [[apps]] (see BenchConfig.framework_app, pilot/config/bench.py:368), so a postgres bench in that state can never save its database settings.

Suggested change
if error := get_framework_branch_error(data.get("db_type", "mariadb"), data.get("app_branch", "")):
return error
if error := get_framework_branch_error(
data.get("db_type", "mariadb"), data.get("app_branch") or POSTGRES_FRAMEWORK_BRANCH
):
return error
Prompt for AI agents
In admin/backend/api/v1/setup/config.py at lines 40-41: Missing app_branch defaults to "" and rejects valid postgres payloads

`data.get("app_branch", "")` turns an absent branch into `""`, so any postgres payload that does not carry `app_branch` now fails with "PostgreSQL benches need Frappe branch 'develop', not ''." — which is why `test_validate_configuration_accepts_postgres_password` had to add `app_branch` to a payload that previously passed. `_update_configuration` merges `current` flat settings, but `read_flat` yields `app_branch == ""` for a bench.toml with no `[[apps]]` (see `BenchConfig.framework_app`, pilot/config/bench.py:368), so a postgres bench in that state can never save its database settings.

Suggested replacement for those lines:
    if error := get_framework_branch_error(
        data.get("db_type", "mariadb"), data.get("app_branch") or POSTGRES_FRAMEWORK_BRANCH
    ):
        return error

Check the claim against the code first. Fix it only if it holds.

Fix with your agent

return _validate_app_source(data)


Expand Down
7 changes: 7 additions & 0 deletions admin/frontend/dashboard/src/composables/setup/useSetup.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ const DB_TYPE_OPTIONS = [
{ label: 'MariaDB', value: 'mariadb' },
{ label: 'PostgreSQL', value: 'postgres' },
]
// Frappe supports PostgreSQL only on develop.
Comment thread
mihir-kandoi marked this conversation as resolved.
const POSTGRES_FRAMEWORK_BRANCH = 'develop'
const STEP_TITLES: Record<string, string> = {
database: 'Database',
customize: 'Customize your bench',
Expand Down Expand Up @@ -95,6 +97,10 @@ export const useSetup = () => {
const dbPortPlaceholder = computed(() => (dbType.value === 'mariadb' ? '3306' : '5432'))
const resolvedDbUser = computed(() => dbUser.value || rootUserPlaceholder.value)

const isBranchLocked = computed(() => dbType.value === 'postgres')
watch(isBranchLocked, (locked) => {
if (locked) appBranch.value = POSTGRES_FRAMEWORK_BRANCH
})
Comment thread
mihir-kandoi marked this conversation as resolved.
const branchOptions = computed(() =>
branchComboboxOptions(availableBranches.value, appBranch.value, (typed: string) => {
appBranch.value = typed
Expand Down Expand Up @@ -427,6 +433,7 @@ export const useSetup = () => {
rootUserPlaceholder,
dbTypeOptions: DB_TYPE_OPTIONS,
branchOptions,
isBranchLocked,
validatingFramework,
frameworkIsValid,
stepSequence,
Expand Down
3 changes: 3 additions & 0 deletions admin/frontend/dashboard/src/pages/setup/Setup.vue
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ const {
rootUserPlaceholder,
dbTypeOptions,
branchOptions,
isBranchLocked,
validatingFramework,
frameworkIsValid,
stepSequence,
Expand Down Expand Up @@ -107,6 +108,8 @@ const {
label="Frappe branch"
v-model="appBranch"
:options="branchOptions"
:disabled="isBranchLocked"
:description="isBranchLocked ? 'PostgreSQL needs the develop branch.' : undefined"
Comment thread
frappe-guard[bot] marked this conversation as resolved.
trigger="button"
placeholder="Search or type a branch…"
>
Expand Down
2 changes: 1 addition & 1 deletion docs/admin-ui.md
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ The UI should treat task ids as the handle for long work.

## Setup

The Frappe branch picker lists the branches reported by the repository. Type a different full branch name and select `Use branch` to choose it. `Set up bench` saves the selection before setup starts. The wizard restores a saved custom branch even when the repository list does not contain it.
The Frappe branch picker lists the branches reported by the repository. Type a different full branch name and select `Use branch` to choose it. `Set up bench` saves the selection before setup starts. The wizard restores a saved custom branch even when the repository list does not contain it. Choosing PostgreSQL locks the picker to `develop`.

## Site Navigation

Expand Down
2 changes: 1 addition & 1 deletion docs/configuration.md
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ queues = ["default", "short", "long"]
count = 1
```

The first app is the framework app. A bench uses one database type for all its sites. Supported types are `mariadb` and `postgres`.
The first app is the framework app. A bench uses one database type for all its sites. Supported types are `mariadb` and `postgres`. A `postgres` bench needs the framework app on `develop`; `pilot init` and setup reject other branches.

## Bench settings

Expand Down
8 changes: 8 additions & 0 deletions pilot/config/bench.py
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,14 @@

# Framework branches the setup wizard offers, newest/recommended first.
FRAMEWORK_BRANCHES = ["version-16", "develop"]
# Frappe supports PostgreSQL only on develop.
POSTGRES_FRAMEWORK_BRANCH = "develop"


def get_framework_branch_error(db_type: str, branch: str) -> str | None:
if db_type == "postgres" and branch != POSTGRES_FRAMEWORK_BRANCH:
return f"PostgreSQL benches need Frappe branch '{POSTGRES_FRAMEWORK_BRANCH}', not '{branch}'."
Comment thread
mihir-kandoi marked this conversation as resolved.
return None

_DEFAULT_DATA: dict = {
"bench": {"name": "", "python": "3.14"},
Expand Down
3 changes: 3 additions & 0 deletions pilot/core/bench/creator.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
from pathlib import Path
from typing import TYPE_CHECKING

from pilot.config.bench import POSTGRES_FRAMEWORK_BRANCH
from pilot.exceptions import BenchAlreadyExistsError
from pilot.utils import iter_sibling_benches

Expand Down Expand Up @@ -80,6 +81,8 @@ def _initial_settings(self) -> dict:
"db_type": self.db_type,
"lite_mode_enabled": True,
}
if self.db_type == "postgres":
settings["app_branch"] = POSTGRES_FRAMEWORK_BRANCH
if self.process_manager:
settings["production_process_manager"] = self.process_manager
return settings
Expand Down
10 changes: 9 additions & 1 deletion pilot/core/bench/initializer.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,9 @@
from collections.abc import Callable
from typing import TYPE_CHECKING

from pilot.config.bench import get_framework_branch_error
from pilot.config.common_site_config import update_common_site_config
from pilot.exceptions import ConfigError

if TYPE_CHECKING:
from pilot.core.bench import Bench
Expand Down Expand Up @@ -40,7 +42,7 @@ def _run_steps(self, on_progress: Callable[[str], None]) -> None:
# by install.sh, and MariaDB/PostgreSQL run as a rootless per-user
# server (see MariaDBManager/PostgresManager).
steps: list[tuple[str, Callable[[], None]]] = [
("Validate bench.toml", self.bench.config.validate),
("Validate bench.toml", self._validate_config),
("Ensure admin password", self._ensure_admin_password),
("Ensure database credentials", self._ensure_database_credentials),
("Install system packages", self._install_system_packages),
Expand All @@ -67,6 +69,12 @@ def _run_steps(self, on_progress: Callable[[str], None]) -> None:
on_progress(" pilot new-site site1.example.com # create your first site")
on_progress(" pilot start # start all processes")

def _validate_config(self) -> None:
config = self.bench.config
config.validate()
if error := get_framework_branch_error(config.db_type, config.framework_app.branch):
raise ConfigError(error)

def _rollback(self, on_progress: Callable[[str], None]) -> None:
if not self._rollback_actions:
return
Expand Down
8 changes: 7 additions & 1 deletion tests/admin/backend/api/test_setup_postgres.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,16 @@ def test_validate_configuration_requires_postgres_password() -> None:


def test_validate_configuration_accepts_postgres_password() -> None:
data = {"admin_password": "x", "db_type": "postgres", "postgres_password": "pw"}
data = {"admin_password": "x", "db_type": "postgres", "postgres_password": "pw", "app_branch": "develop"}
assert validate_configuration(data) is None


def test_validate_configuration_rejects_postgres_off_develop() -> None:
data = {"admin_password": "x", "db_type": "postgres", "postgres_password": "pw", "app_branch": "version-16"}
error = validate_configuration(data)
assert error and "develop" in error


def test_validate_configuration_requires_mariadb_password() -> None:
error = validate_configuration({"admin_password": "x", "db_type": "mariadb", "mariadb_password": ""})
assert error and "mariadb_password" in error
Expand Down
11 changes: 10 additions & 1 deletion tests/e2e/flows/wizard.py
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,9 @@ def complete_dev_wizard(
# choice - that's a separate `pilot setup production` step run from the terminal
# afterwards). We keep the repo default.
expect(page.get_by_text("Customize your bench")).to_be_visible(timeout=30_000)
if framework_branch:
if db_type == "postgres":
_expect_branch_locked(page)
elif framework_branch:
_choose_combobox_button(page, "Frappe branch", framework_branch)

page.get_by_role("button", name="Set up bench").click()
Expand Down Expand Up @@ -80,6 +82,13 @@ def _choose_select(page: Page, label: str, option_name: str) -> None:
page.get_by_role("option", name=option_name).click()


def _expect_branch_locked(page: Page) -> None:
"""PostgreSQL benches are locked to the develop branch."""
picker = page.get_by_role("button", name="Frappe branch")
expect(picker).to_be_disabled()
expect(picker).to_contain_text("develop")


def _choose_combobox_button(page: Page, label: str, option_name: str) -> None:
"""Choose an option in a Frappe UI Combobox with `trigger="button"`."""
page.get_by_role("button", name=label).click()
Expand Down
1 change: 1 addition & 0 deletions tests/pilot/commands/test_commands.py
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,7 @@ def test_new_command_postgres_bench_has_no_password_yet(
with open(benches_dir / "pg" / "bench.toml", "rb") as f:
data = tomllib.load(f)
assert data["bench"]["db_type"] == "postgres"
assert data["apps"][0]["branch"] == "develop"
assert not CommonConfig.read(benches_dir).postgres.root_password


Expand Down
9 changes: 9 additions & 0 deletions tests/pilot/commands/test_init_command.py
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,15 @@ def test_raises_when_existing_credentials_are_wrong() -> None:
manager.provision.assert_not_called()


def test_rejects_postgres_bench_off_develop() -> None:
bench = MagicMock()
bench.config.db_type = "postgres"
bench.config.framework_app.branch = "version-16"

with pytest.raises(BenchError, match="develop"):
BenchInitializer(bench)._validate_config()


def test_ensure_database_credentials_generates_for_a_fresh_server() -> None:
bench = MagicMock()
bench.config.db_type = "mariadb"
Expand Down
Loading