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

feat: support extracting a module config from the local environment #1331

Merged
merged 1 commit into from
Apr 25, 2024

Conversation

alecthomas
Copy link
Collaborator

@alecthomas alecthomas commented Apr 25, 2024

This is intended primarily for local development/testing. Currently now this will load from ftl-project.toml and environment variables, but at some point the envars will be removed.

This also moves all ModuleContext related code into go-runtime/modulecontext.

@alecthomas alecthomas requested a review from a team as a code owner April 25, 2024 05:06
@alecthomas alecthomas requested review from matt2e and removed request for a team April 25, 2024 05:06
This is intended primarily for local development/testing. Currently now this
will load from ftl-project.toml and environment variables, but at some
point the envars will be removed.
@alecthomas alecthomas mentioned this pull request Apr 25, 2024
@alecthomas alecthomas force-pushed the aat/module-context-from-environment branch from e2a8063 to 1d083b8 Compare April 25, 2024 05:07
@@ -84,6 +85,9 @@ func loadFile(path string) (Config, error) {
config := Config{}
md, err := toml.DecodeFile(path, &config)
if err != nil {
if errors.Is(err, os.ErrNotExist) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This allows ftl config to operate when the ftl-project.toml file doesn't exist yet.

@alecthomas alecthomas enabled auto-merge (squash) April 25, 2024 05:10
@alecthomas alecthomas merged commit b4340ea into main Apr 25, 2024
11 checks passed
@alecthomas alecthomas deleted the aat/module-context-from-environment branch April 25, 2024 05:11
)

// Module returns the FTL module currently being executed.
func Module() string {
Copy link
Collaborator

Choose a reason for hiding this comment

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

👏 I was wanting this more generaly and didnt want to call config.go's callerModule()
And it seems that pfi wanted it in some cases as well: https://github.com/tbdeng/pfi/blob/ef7ee116ca4dc269dc9cc857187e2b0f5e697489/backend/modules/tbdex/offerings/offerings.go#L17

@@ -647,14 +648,18 @@ func (s *Service) GetModuleContext(ctx context.Context, req *connect.Request[ftl
if err != nil {
return nil, connect.NewError(connect.CodeInternal, fmt.Errorf("could not get active schemas: %w", err))
}
schema, ok := slices.Find(schemas, func(s *schema.Module) bool { return s.Name == req.Msg.Module })
module, ok := slices.Find(schemas, func(s *schema.Module) bool { return s.Name == req.Msg.Module })
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like configs, secrets and DSNs are now discovered without needing the declarations from a module. So the db query to find module can be skipped

Copy link
Collaborator

Choose a reason for hiding this comment

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

Now this module var is only used to get the module name, which can just get directly from the req

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah another thing on my mind: when should a deployment fail if a config/secret/DSN is missing. I think that was the other reason i was getting the module, so that we could fail earlier.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah it should fail at startup.

@matt2e matt2e added the approved Marks an already closed PR as approved label Apr 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Marks an already closed PR as approved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants