Various fixes for targeting postgres in singularity (rebase) - #1679
Open
jmchilton wants to merge 19 commits into
Open
Various fixes for targeting postgres in singularity (rebase)#1679jmchilton wants to merge 19 commits into
jmchilton wants to merge 19 commits into
Conversation
This was referenced Aug 20, 2026
mvdbeek
reviewed
Aug 22, 2026
mvdbeek
reviewed
Aug 22, 2026
mvdbeek
reviewed
Aug 22, 2026
mvdbeek
approved these changes
Aug 22, 2026
jmchilton
added a commit
to jmchilton/planemo
that referenced
this pull request
Aug 23, 2026
…p-dir the tests - MANAGED_DATABASE_TYPES comment pointed at galaxyproject#1679, which is the PR itself. - _database_connection takes database_connection/database_type by name instead of digging them out of kwds; the docstring no longer repeats the comment. - The unit tests build their database location under mkdtemp rather than a hard-coded /tmp path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
It'll be stopped via the `_database_connection` context manager
_create_profile_local's sqlite branch called database_source.create_database(), but database_source and database_identifier are only bound when the type is not sqlite, so `profile_create --database_type sqlite` raised UnboundLocalError. create_database_source has no sqlite branch either - it is a TODO that raises "Unknown database type [sqlite]" - so restore the DATABASE_LOCATION_TEMPLATE path instead, which also puts the now-unused import back to work. Drop the dead `database_connection + database_source.sqlalchemy_url(...)` statement while here: `+` where `=` was meant, on names unbound in that branch, and unreachable now that postgres_singularity no longer skips the block above. Add a sqlite case to the profile command tests - both existing cases are gated on postgres/docker, so nothing covered this path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
create_database_source grew a profile_directory parameter that selects the singularity container's on-disk location, but no call site passed it, so it was always None and the location fell back to a throwaway mkdtemp - the database was not stored in the profile despite the option existing to do so. Pass it from _create_profile_local and delete_profile, which both have it in scope and must agree on where the database lives. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
_build_env_for_galaxy copies properties into a fresh dict, so assigning properties["database_connection"] after it left GALAXY_CONFIG_OVERRIDE_DATABASE_CONNECTION unset - what tests/test_galaxy_config.py::test_defaults asserts on. Open the _database_connection context manager where master assigned the property instead, above the env build.
_database_connection tested `database_type != "sqlite"`, but the option defaults to "auto" and most callers never set it at all, so the default path built a DatabaseSource. create_database_source resolves "auto" by probing for psql, and LocalPostgresDatabaseSource has no user configured by default - a bare `planemo serve` handed Galaxy postgresql://None@localhost/galaxy and hung until CI timed out. Name the managed types in MANAGED_DATABASE_TYPES so which types get a server is one line rather than an inverted test, and honour an explicit database_connection again - dropping it also cut profiles off from the connection string they had resolved and stored.
Moving container startup out of DockerPostgresDatabaseSource.__init__ into an explicit start() is right, but only _database_connection was taught the new lifecycle. database_list/create/delete and profile creation all built a source and used it cold, so postgres_docker gave "No such container: planemopostgres". started_database_source() constructs and starts in one call for those callers. It deliberately does not stop: the container runs with --rm, so shutting it down would discard the database just created. Runs that own a Galaxy keep using create_database_source and manage stop() themselves.
create_database/delete_database were stubbed out as no-ops so that profile setup would not touch the database, but they live on the shared DatabaseSource that the user-facing database_create and database_delete commands drive - so database_create printed "Database with URL ... created." having created nothing, and test_profile_commands_docker passed vacuously. create_database also came off the ABC while three call sites still used it; put it back. Restore the try/finally container cleanup in both docker tests. The commit that dropped it reasoned the container would be stopped by _database_connection, but that only covers a Galaxy run - the database_* and profile_create paths deliberately leave it up.
…p-dir the tests - MANAGED_DATABASE_TYPES comment pointed at galaxyproject#1679, which is the PR itself. - _database_connection takes database_connection/database_type by name instead of digging them out of kwds; the docstring no longer repeats the comment. - The unit tests build their database location under mkdtemp rather than a hard-coded /tmp path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
which("psql") proves the client is installed, not that a server is reachable -
--database_type postgres is documented as "an existing postgres server you user
can access without a password via the psql command". So the auto ladder in
create_database_source and _create_profile_local was choosing a backend on
evidence that did not support the choice.
auto now means sqlite for a profile, and the database_* commands, which have no
sqlite backend to fall back to, ask for a --database_type instead of guessing.
The RuntimeError-to-sqlite fallback in _create_profile_local goes with it: it
was only reachable through auto, and an explicitly named backend that fails to
create its database should say so.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NtfpiZ43Gk56tUyoRGizC8
jmchilton
force-pushed
the
postgres_singularity_fix_rebased
branch
from
August 29, 2026 15:06
25613c1 to
e2f950d
Compare
Create and drop named databases through containerized PostgreSQL tools, persist profile identity and storage, and keep the database alive until Galaxy exits. Add bounded readiness and shutdown handling plus focused regression and opt-in acceptance coverage.
Ensure direct managed backends create their target database, isolate profile command tests, and remove partial profiles after creation failures. Centralize backend registration and profile metadata, give daemon shutdown a single owner, and refresh generated command documentation.
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
2 participants
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.
Rebase and fixes for #1557