Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: error out if specified config doesn't exist, fixes #1347 #1485

Merged
merged 2 commits into from
May 15, 2024
Merged

fix: error out if specified config doesn't exist, fixes #1347 #1485

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6e6ff0a
fix: error out if specified config doesn't exist, fixes #1347
May 15, 2024
2399f9d
Merge branch 'main' into config-non-existent-error
safeer May 15, 2024
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: 1 addition & 2 deletions cmd/ftl/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package main

import (
"context"
"errors"
"net/url"
"os"
"os/signal"
Expand Down Expand Up @@ -81,7 +80,7 @@ func main() {
ctx = log.ContextWithLogger(ctx, logger)

config, err := projectconfig.LoadConfig(ctx, cli.ConfigFlag)
if err != nil && !errors.Is(err, os.ErrNotExist) {
if err != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

The semantics are a little more involved than this will allow for:

  1. If no config is passed on the CLI, we should try to load ftl-project.toml from the root of the repo, but if it doesn't exist that's okay.
  2. If a config is passed explicitly on the command-line, and doesn't exist, this should error.

The simplest way to do this is probably to just check if cli.ConfigFlag == ""

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm probably missing something, but I tried these cases with the following results:

  1. No config passed to CLI and ftl-project.toml exists in the git root: succeeds, has populated Config.
  2. No config passed to CLI and ftl-project.toml doesn't exist in the git root: succeeds, has empty Config.
  3. Config passed to CLI that doesn't exist: error.

Relevant code is in ConfigPaths which returns the config passed to CLI if it exists, the ftl-project.toml in the git root if it exists, otherwise an empty slice. The subsequent call to Merge fails if a path doesn't exist, but it's never provided a nonexistent ftl-project.toml in the git root.

I went down the path of testing by mocking out GetDefaultConfigPath() but that got unwieldy for this change 😅

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah awesome, apologies I forgot about that code in ConfigPaths, you're 100% right! LGTM

kctx.Fatalf(err.Error())
}
kctx.Bind(config)
Expand Down
4 changes: 0 additions & 4 deletions common/projectconfig/projectconfig.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package projectconfig

import (
"context"
"errors"
"fmt"
"os"
"path/filepath"
Expand Down Expand Up @@ -85,9 +84,6 @@ func loadFile(path string) (Config, error) {
config := Config{}
md, err := toml.DecodeFile(path, &config)
if err != nil {
if errors.Is(err, os.ErrNotExist) {
return Config{}, nil
}
return Config{}, err
}
if len(md.Undecoded()) > 0 {
Expand Down
23 changes: 23 additions & 0 deletions common/projectconfig/projectconfig_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,29 @@ func TestProjectConfig(t *testing.T) {
assert.Equal(t, expected, actual)
}

func TestProjectLoadConfig(t *testing.T) {
tests := []struct {
name string
paths []string
err string
}{
{name: "AllValid", paths: []string{"testdata/ftl-project.toml"}},
{name: "IsNonExistent", paths: []string{"testdata/ftl-project-nonexistent.toml"}, err: "no such file or directory"},
{name: "ContainsNonExistent", paths: []string{"testdata/ftl-project.toml", "testdata/ftl-project-nonexistent.toml"}, err: "no such file or directory"},
}

for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
_, err := LoadConfig(log.ContextWithNewDefaultLogger(context.Background()), test.paths)
if test.err != "" {
assert.Contains(t, err.Error(), test.err)
} else {
assert.NoError(t, err)
}
})
}
}

func TestProjectConfigChecksMinVersion(t *testing.T) {
tests := []struct {
name string
Expand Down
Loading