-
Notifications
You must be signed in to change notification settings - Fork 8
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.
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
Conversation
@@ -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 { |
There was a problem hiding this comment.
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:
- 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. - 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 == ""
There was a problem hiding this comment.
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:
- No config passed to CLI and
ftl-project.toml
exists in the git root: succeeds, has populated Config. - No config passed to CLI and
ftl-project.toml
doesn't exist in the git root: succeeds, has empty Config. - 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 😅
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice!!! :D
See https://github.com/TBD54566975/ftl/actions/runs/9102976431/job/25023813425 Revert "fix: error out if specified config doesn't exist, fixes #1347 (#1485)" This reverts commit 57659f3.
…sts (#1504) See https://github.com/TBD54566975/ftl/actions/runs/9102976431/job/25023813425 I'm going to pull the integration tests into PR CI, unfortunately it's really opaque when it only runs on main. Revert "fix: error out if specified config doesn't exist, fixes #1347 (#1485)" This reverts commit 57659f3.
No description provided.