feat: module context over gRPC #1311

matt2e · 2024-04-19T06:21:22Z

Doc: https://hackmd.io/@ftl/SyynEK2eC
Other PRs

Pre-Req: protobuf changes feat: module context over gRPC (protobuf changes) #1312
Possible future change for tests moved here: feat: module tests can declare config/secrets/DSNs and add verb mocks #1317

Changes:

Module server requests module context, instead of calculating them independently from envars
controller alculates values for module context (config, secrets, DSNs), returns it to module via gRPC
Within a module, ftl.PostgresDatabase(name string) now returns a handler, with Get() needing to be called to get *sql.DB
Module context builder is used to compose a module context. It can take in:
- a GetModuleContext response, to read in values received over gRPC
- manual entry (useful for testing) (see other PR above)
Added in-memory implementations of configuration.MutableProvider and configuration.Resolver. This allows us to have a backing store for configs and secrets for ModuleContext which shares the same behaviour for (un)marshalling json.
- configuration.Manager now has raw data versions of Get() and Set() called GetData() and SetData() which expose the features without needing unnecessary conversion steps. This helps when moving values between configuration managers directly or over gRPC.

Smaller PR for this: #1311 Just includes protobuf changes

alecthomas · 2024-04-22T02:40:04Z

common/modulecontext/builder.go

@@ -0,0 +1,254 @@
+package modulecontext


Please add comments to the public functions/types in here.

common/modulecontext/builder.go

alecthomas · 2024-04-22T02:53:02Z

common/modulecontext/builder.go

+		return "", fmt.Errorf("missing environment variable %q", r.name)
+	}
+	return value, nil
+}


The builder feels like a lot of machinery to do what seems like it should be fairly straightforward? Currently there's only a config manager, a secret manager, and DB envars. Can all this code just be a single function that takes those and outputs a ModuleContext?

Moved this to just a helper function in the controller packager rather than using modulecontext.
It's in a separate file to avoid bloating controller.go more

alecthomas · 2024-04-22T02:57:23Z

common/modulecontext/modulecontext.go

@@ -0,0 +1,96 @@
+package modulecontext


I think there's too much mixing of concerns in this package - it is used both on the server side and the client side. That needs to be split out - the server should be very straightforward, just converting state to the proto and sending it, while the client should be most of what is in this package now, with any server-side code removed.

Yeh agreed.
Simplified it a lot after removing modulecontext from the serverside, and removing the difference between global / non global.
There might be use still for data vs value when we have testing, but that's no longer in this PR anyway, so I've simplified that away as well.

alecthomas

A few more minor comments, but overall LGTM.

alecthomas · 2024-04-22T05:10:52Z

cmd/ftl/main.go

-	// Propagate to runner processes.
-	// TODO: This is a bit of a hack until we get proper configuration
-	// management through the Controller.
-	os.Setenv("FTL_CONFIG", strings.Join(projectconfig.ConfigPaths(cli.ConfigFlag), ","))


alecthomas · 2024-04-22T05:13:30Z

common/modulecontext/builder.go

+	}
+}
+
+func (b *Builder) Build(ctx context.Context) (*ModuleContext, error) {


Given there are now no "builder" methods, I'd just make this a single global function modulecontext.FromProto() (*ModuleContext, error) and remove anything Builder related as it seems like an unnecessary abstraction for now. We can revisit once we get around to fleshing out the testing story.

moved the builder to the test PR

common/modulecontext/builder.go

alecthomas · 2024-04-22T05:15:12Z

go-runtime/ftl/database.go

+// Get returns the sql db connection for the database.
+func (d Database) Get(ctx context.Context) *sql.DB {
+	provider := modulecontext.DBProviderFromContext(ctx)
+	db, err := provider.GetDB(d.Name)


Nice I like this.

common/modulecontext/db_provider.go

matt2e requested a review from a team as a code owner April 19, 2024 06:21

matt2e requested review from wesbillman and removed request for a team April 19, 2024 06:21

alecthomas mentioned this pull request Apr 19, 2024

Dashboard #728

Open

matt2e mentioned this pull request Apr 19, 2024

feat: module context over gRPC (protobuf changes) #1312

Merged

matt2e marked this pull request as draft April 19, 2024 06:22

matt2e force-pushed the matt2e/config-over-grpc branch 3 times, most recently from d0b529d to 8a62999 Compare April 19, 2024 06:28

matt2e changed the title ~~feat: config over grpc~~ feat: module context over gRPC Apr 19, 2024

matt2e force-pushed the matt2e/config-over-grpc branch from 8a62999 to 56866f3 Compare April 19, 2024 06:50

matt2e linked an issue Apr 19, 2024 that may be closed by this pull request

Replace ad-hoc envar "protocol" with something more structured #1240

Closed

matt2e changed the title ~~feat: module context over gRPC~~ [DRAFT] feat: module context over gRPC Apr 19, 2024

matt2e mentioned this pull request Apr 21, 2024

Is there a way to specify which project toml to use in tests? #1309

Closed

github-actions bot changed the title ~~[DRAFT] feat: module context over gRPC~~ feat: module context over gRPC Apr 22, 2024

matt2e force-pushed the matt2e/config-over-grpc branch from 4d4dbc5 to 3232724 Compare April 22, 2024 00:31

matt2e added a commit that referenced this pull request Apr 22, 2024

feat: module context over gRPC (protobuf changes) (#1312)

4f055de

Smaller PR for this: #1311 Just includes protobuf changes

matt2e force-pushed the matt2e/config-over-grpc branch 2 times, most recently from 95786d2 to ae2b543 Compare April 22, 2024 01:10

matt2e mentioned this pull request Apr 22, 2024

feat: module tests can declare config/secrets/DSNs and add verb mocks #1317

Merged

alecthomas reviewed Apr 22, 2024

View reviewed changes

matt2e force-pushed the matt2e/config-over-grpc branch from f65f327 to 1929079 Compare April 22, 2024 03:08

matt2e changed the title ~~feat: module context over gRPC~~ [Draft] feat: module context over gRPC Apr 22, 2024

github-actions bot changed the title ~~[Draft] feat: module context over gRPC~~ feat: draft module context over gRPC Apr 22, 2024

matt2e force-pushed the matt2e/config-over-grpc branch from b290154 to 7cff44e Compare April 22, 2024 05:03

alecthomas approved these changes Apr 22, 2024

View reviewed changes

matt2e changed the title ~~feat: draft module context over gRPC~~ feat: module context over gRPC Apr 22, 2024

matt2e added 3 commits April 22, 2024 16:40

feat: module context over gRPC

a464fdc

add builder test

8e24780

make builders mutable so chaining not required

f06ef41

matt2e added 7 commits April 22, 2024 16:40

serverside no longer uses modulecontext directly

a581616

module context proto doesn’t need to care if global or not

43f2fdd

add comments

03f4513

simplify db provider func names

088acdc

move modulecontext to go-runtime

95f6017

fix errors and lint

3e761c3

simplify by removing builder

30da785

matt2e force-pushed the matt2e/config-over-grpc branch from 5d66e67 to 30da785 Compare April 22, 2024 06:40

matt2e marked this pull request as ready for review April 22, 2024 06:41

matt2e merged commit a916327 into main Apr 22, 2024
11 checks passed

matt2e deleted the matt2e/config-over-grpc branch April 22, 2024 06:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: module context over gRPC #1311

feat: module context over gRPC #1311

matt2e commented Apr 19, 2024 •

edited

Loading

alecthomas Apr 22, 2024

alecthomas Apr 22, 2024

matt2e Apr 22, 2024

alecthomas Apr 22, 2024

matt2e Apr 22, 2024

alecthomas left a comment

alecthomas Apr 22, 2024

alecthomas Apr 22, 2024

matt2e Apr 22, 2024

alecthomas Apr 22, 2024

feat: module context over gRPC #1311

feat: module context over gRPC #1311

Conversation

matt2e commented Apr 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alecthomas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matt2e commented Apr 19, 2024 •

edited

Loading