-
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: don't attempt to create ftl-project.toml in fs root #1771
Conversation
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! Thanks for taking a stab at this one!
cmd/ftl/cmd_init.go
Outdated
@@ -71,7 +71,7 @@ func (i initGoCmd) Run(ctx context.Context, parent *initCmd) error { | |||
if err := updateGitIgnore(i.Dir); err != nil { | |||
return err | |||
} | |||
if err := projectconfig.MaybeCreateDefault(ctx); err != nil { | |||
if err := projectconfig.MaybeCreateDefault(ctx, i.Dir); 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.
I think we might want to create the project file in the gitRoot folder vs. where this command was run. I just tried this in the FTL repo and it added the project file to examples/go
since that's where I ran the ftl init....
command. But I would guess we would want the file to be created at the root of the ftl repo.
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 agree; think we want the search for ftl-project.toml
to end at the gitroot instead of walking up the tree to the fs root, then it would create it there if it doesn't exist. Was planning on bringing this up during standup, but I'm also wondering about the general assumption that the project is in a git repo. My test project wasn't and FTL was very angry.
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.
@wesbillman added an explicit check for a git repo during init
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. Thanks!
Maybe we should just list the assumptions and make sure the code does what we expect. So maybe the process is something like this in order (please feel free to make changes or add assumptions here)
- Check for existing ftl-project.toml.
- Create new ftl-project.toml at the gitRoot if gitRoot is found
- Create new ftl-project at the location where the
ftl init
command was run
I don't suspect we would ever want to create this at the FS root.
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.
Chatted with @safeer and he's going to put together an RFC for changing the CLI to have more obvious commands for creating ftl projects and modules.
4f3902f
to
250dfe0
Compare
Fixes #1770