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.

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

chore: dev cmd can read directories from project toml #1126

Merged
merged 14 commits into from
Mar 24, 2024

Conversation

matt2e
Copy link
Collaborator

@matt2e matt2e commented Mar 22, 2024

Issue: #1125

@alecthomas alecthomas mentioned this pull request Mar 22, 2024
cmd/ftl/cmd_dev.go Outdated Show resolved Hide resolved
@matt2e
Copy link
Collaborator Author

matt2e commented Mar 22, 2024

Should ftl build also default to the project toml file's directories?

@alecthomas
Copy link
Collaborator

Should ftl build also default to the project toml file's directories?

Yes

@matt2e matt2e force-pushed the matt2e/directories-in-project-toml branch from 50cb168 to 7f39ded Compare March 22, 2024 04:11
@matt2e matt2e marked this pull request as ready for review March 22, 2024 04:11
cmd/ftl/main.go Outdated
@@ -83,6 +83,9 @@ func main() {
kctx.BindTo(sr, (*cf.Resolver[cf.Secrets])(nil))
kctx.BindTo(cr, (*cf.Resolver[cf.Configuration])(nil))

config, _ := cr.LoadConfig(ctx)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is using the internal state of the ProjectConfigResolver. Instead move this function into the projectconfig directory as a top-level function, and pass it into ProjectConfigResolver (and Bind())

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've now moved it to load it in separately but I'm unsure about passing it in to the resolver like we'd discussed.
Looking at the implementation, every read/write of config is loaded/written and theres no Config held in memory.

If we pass in Config to the resolvers, that would mean changing implementations so that we keep Config in memory and read from memory instead of from disk. And also seems weird to me that it would accept a Config at initialization, but when calling Set/Unset it would write to a file, which you'd hope is where the initial Config originated.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Makes sense. Centralise the saving logic in the projectconfig package too then?

@matt2e matt2e force-pushed the matt2e/directories-in-project-toml branch 2 times, most recently from 00a6cea to 4c23bbf Compare March 24, 2024 21:51
@matt2e matt2e requested a review from alecthomas March 24, 2024 21:55
@matt2e matt2e force-pushed the matt2e/directories-in-project-toml branch from e582434 to 21450bc Compare March 24, 2024 22:35
return config, nil
}

func LoadWritableConfig(ctx context.Context, input []string) (Config, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this should have a comment...

@matt2e matt2e merged commit 61fe56d into main Mar 24, 2024
10 checks passed
@matt2e matt2e deleted the matt2e/directories-in-project-toml branch March 24, 2024 22:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants