fix: allow for postgres image to be specified #2158
Conversation
stuartwdouglas
requested review from
a team and
worstell
and removed request for
a team
cmd/ftl/cmd_serve.go
Outdated
| @@ -44,6 +44,7 @@ type serveCmd struct { | |||
| Stop bool `help:"Stop the running FTL instance. Can be used with --background to restart the server" default:"false"` | |||
| StartupTimeout time.Duration `help:"Timeout for the server to start up." default:"1m"` | |||
| ObservabilityConfig observability.Config `embed:"" prefix:"o11y-"` | |||
| DatabaseImage string `help:"The container image to start for the database" default:"postgres:15.4" env:"FTL_DATABASE_IMAGE"` | |||
There was a problem hiding this comment.
This still needs some work, it seems like something is still starting the old container
There was a problem hiding this comment.
Would postgres:latest be a better default? (not sure if that even works)
There was a problem hiding this comment.
The linked ticket was about aligning versions with the production database. One issue with using latest in general is that versions can change underneath you, meaning that code that previously worked can suddenly start to fail.
stuartwdouglas
force-pushed
the
stuartwdouglas/2111
branch
6 times, most recently
from
512dc3b to
34c4f1b
Compare
internal/container/container.go
Outdated
| } | ||
| for _, container := range containers { | ||
| if container.Image != image { | ||
| return false, fmt.Errorf("expecting to use container image %s for container with name %s, bit it was already running with image %s. please delete the container or select a different database image", image, name, container.Image) |
There was a problem hiding this comment.
Go error messages need to be composable by concatenation, so they shouldn't include full stops or full sentences (in general). Instead use ",", ":" or ";" or similar.
| return false, fmt.Errorf("expecting to use container image %s for container with name %s, bit it was already running with image %s. please delete the container or select a different database image", image, name, container.Image) | |
| return false, fmt.Errorf("expecting to use container image %s for container with name %s, bit it was already running with image %s, please delete the container or select a different database image", image, name, container.Image) |
cmd/ftl/cmd_serve.go
Outdated
| @@ -44,6 +44,7 @@ type serveCmd struct { | |||
| Stop bool `help:"Stop the running FTL instance. Can be used with --background to restart the server" default:"false"` | |||
| StartupTimeout time.Duration `help:"Timeout for the server to start up." default:"1m"` | |||
| ObservabilityConfig observability.Config `embed:"" prefix:"o11y-"` | |||
| DatabaseImage string `help:"The container image to start for the database" default:"postgres:15.4" env:"FTL_DATABASE_IMAGE"` | |||
There was a problem hiding this comment.
Why do we want this to be configurable? FTL itself will require a specific (minimum, probably) DB version, which shouldn't be configurable in general by users. I can see it being useful for debugging though, so maybe it could be a hidden:"" flag?
There was a problem hiding this comment.
I figured it would be useful for debugging, if we end up with production environments with different versions installed.
| @@ -105,28 +115,26 @@ func Run(ctx context.Context, image, name string, hostPort, containerPort int, v | |||
| } | |||
|
|
|||
| // RunDB runs a new detached postgres container with the given name and exposed port. | |||
| func RunDB(ctx context.Context, name string, port int) error { | |||
| func RunDB(ctx context.Context, name string, port int, image string) error { | |||
There was a problem hiding this comment.
We lean on Option[T] heavily in the codebase where semantically relevant, rather than implicit zero values, so let's replace this with optional.Option[string] to make it clear that it can be omitted.
There was a problem hiding this comment.
Note that RunDB does actually need this, it is DoesExist where it is optional
internal/container/container.go
Outdated
| @@ -37,8 +37,18 @@ func DoesExist(ctx context.Context, name string) (bool, error) { | |||
| if err != nil { | |||
| return false, fmt.Errorf("failed to list containers: %w", err) | |||
| } | |||
|
|
|||
| return len(containers) > 0, nil | |||
| if len(containers) > 0 { | |||
There was a problem hiding this comment.
Style comment: we practice return-early-return-often, so in this case we'd invert a few of these if cases and unindent.
stuartwdouglas
force-pushed
the
stuartwdouglas/2111
branch
from
34c4f1b to
51cf4aa
Compare
|
Thinking about it I might just make the error on wrong DB version a warning, otherwise it is going to break peoples development environments and it is likely not worth it at this stage. |
Also change the default to 15.4 to match production fixes #2111
stuartwdouglas
force-pushed
the
stuartwdouglas/2111
branch
from
51cf4aa to
4bad5c1
Compare
Also change the default to 15.4 to match production
fixes #2111