feat: separate out language logic from moduleconfig #3016

matt2e · 2024-10-07T03:28:13Z

Why

Language support was previously tightly integrated with how a ModuleConfig is created. Without this defaulting, FTL does not know key info about a module like deploy directory or where to find the schema.

Another complication is that module defaults for jvm modules is done by looking for certain files to automatically choose defaults for gradle or maven, so we can't get the defaults once per language and use them for all modules of that language.

Creating a ModuleConfig

To create a ModuleConfig, the steps are:

// all error checking is skipped for clarity

// first load the toml file to get a UnvalidatedModuleConfig,
// which holds only the values that were in the toml file.
partialConfig, err := moduleconfig.LoadConfig(dir)

// from this we know the language, and can create a language plugin
plugin, err := languageplugin.New(ctx, uncheckedConfig.Language)

// we then ask the language plugin for the custom defaults
customDefaults, err := plugin.ModuleConfigDefaults(ctx, uncheckedConfig.Dir)

// we can now fill in the defaults and validate the config
config, err := uncheckedConfig.FillDefaultsAndValidate(customDefaults)

Other changes

Removed ModuleGoConfig , ModuleKotlinConfig, ModuleJavaConfig, replace with LanguageConfig: map[string]any
- To do this, we unmarshal the toml into a map[string]any and then parse that into an UnvalidatedModuleConfig using mapstructure and then pull out the language config from the correct key (go, java, kotlin, etc)
buildengine caches the defaults per module so that each time it reads the toml it does not to ask for CustomDefaults from the plugin.

matt2e · 2024-10-07T03:30:55Z

backend/controller/admin/local_client.go

-		}
+		go func() {
+			// Loading a plugin can be expensive. Is there a better way?
+			plugin, err := languageplugin.New(ctx, m.Language)


This change will eventually (once language plugins operate over gRPC) lead to a slowdown in this logic because now we need to create a language plugin per module just so we can figure out where the schema is saved.

I'm not sure of a good solution to this, other than having a central place FTL saves schemas (similar to the central /.ftl/ external modules) so that having a ModuleConfig is not readed to read the saved schema for a module.

Not specifically about the slowdown, but the schema won't be saved right? It will be returned over gRPC?

Plugins don't save the schema, it gets sent over gRPC. What is unresolved at the moment though is how FTL deals with the schema after that. This admin logic is something that breaks if the schema doesn't get saved at all. For now FTL itself is saving the schema just so this and the current deploy logic works and I'll be coming back to this admin stuff a bit later. I've added a section on deployment to the things to do in this main issue here: #2452

matt2e · 2024-10-07T03:31:36Z

internal/buildengine/deps.go

-
-// UpdateDependencies finds the dependencies for a module and returns a
-// Module with those dependencies populated.
-func UpdateDependencies(ctx context.Context, module Module, plugin LanguagePlugin) (Module, error) {


this was unused after previous language plugin changes

matt2e · 2024-10-07T03:32:07Z

internal/buildengine/deps_test.go

-	"github.com/alecthomas/assert/v2"
-)
-
-func TestExtractModuleDepsGo(t *testing.T) {


these have been moved to unit tests within languageplugin

matt2e · 2024-10-07T03:33:26Z

internal/buildengine/languageplugin/go_plugin_helpers.go

+	"golang.org/x/mod/modfile"
+)
+
+func replacementWatches(moduleDir, deployDir string) ([]string, error) {


all this code was previously in moduleconfig but is all specific to the go plugin

matt2e · 2024-10-07T03:34:40Z

internal/buildengine/languageplugin/java_plugin.go

+		GeneratedSchemaDir: "src/main/ftl-module-schema",
+		Deploy:             []string{"launch", "quarkus-app"},
+		// Watch defaults to files related to maven and gradle
+		Watch: []string{"pom.xml", "src/**", "build/generated", "target/generated-sources"},


Previously watch was different based on maven/gradle, but I think it's fine to just use the same watch patterns for both

matt2e · 2024-10-07T03:36:58Z

internal/watch/discover.go

@@ -13,8 +13,8 @@ import (

 // DiscoverModules recursively loads all modules under the given directories
 // (or if none provided, the current working directory is used).
-func DiscoverModules(ctx context.Context, moduleDirs []string) ([]moduleconfig.ModuleConfig, error) {
-	out := []moduleconfig.ModuleConfig{}
+func DiscoverModules(ctx context.Context, moduleDirs []string) ([]moduleconfig.UnvalidatedModuleConfig, error) {


Note: Discover handles UnvalidatedModuleConfigs now, instead of ModuleConfigs

matt2e · 2024-10-07T03:38:35Z

internal/watch/discover_test.go

@@ -14,162 +14,31 @@ func TestDiscoverModules(t *testing.T) {
 	ctx := log.ContextWithNewDefaultLogger(context.Background())
 	modules, err := DiscoverModules(ctx, []string{"testdata"})
 	assert.NoError(t, err)
-	expected := []moduleconfig.ModuleConfig{
+	expected := []moduleconfig.UnvalidatedModuleConfig{


Previously this test was testing discovery and defaulting.
Now it just tests discovery. Generic defaulting tests can now be found in moduleconfig tests, while language specific default testing can be found in languageplugin

matt2e commented Oct 7, 2024

View reviewed changes

ftl-robot mentioned this pull request Oct 7, 2024

Dashboard #728

Open

matt2e commented Oct 7, 2024

View reviewed changes

matt2e force-pushed the matt2e/language-config branch from 75c8e11 to 1de7d64 Compare October 7, 2024 03:51

matt2e marked this pull request as ready for review October 7, 2024 03:59

matt2e requested review from a team and alecthomas as code owners October 7, 2024 03:59

matt2e requested review from wesbillman and removed request for a team October 7, 2024 03:59

matt2e mentioned this pull request Oct 7, 2024

Plugin system for language runtimes #2452

Open

38 tasks

matt2e force-pushed the matt2e/language-config branch from 1f5df72 to b7fbd5f Compare October 8, 2024 00:49

matt2e and others added 12 commits October 8, 2024 13:59

feat: move language specific logic out of moduleconfig

4dd9729

fix test with relative path

f7d6b2e

clean up tests

8a87357

rename DefaultAndValidate func

a662995

add docs

96b12dd

chore(autofmt): Automated formatting

00967b8

fix disk schema retreiver

10044ce

fix moved relative paths to testdata

c2487a4

fix panic on early exit

30f0a41

chore(autofmt): Automated formatting

9f53ec6

do not check ordering of watch list

c2e5b40

fix lint

9b556c4

matt2e added 2 commits October 8, 2024 14:00

fix diff

363dff6

fix TestGeneratedTypeRegistry

c56ce91

matt2e force-pushed the matt2e/language-config branch from a57e9ce to c56ce91 Compare October 8, 2024 03:01

matt2e and others added 2 commits October 8, 2024 14:29

fix schema diff

56d5784

chore(autofmt): Automated formatting

0865e80

matt2e merged commit 28ac330 into main Oct 8, 2024
95 checks passed

matt2e deleted the matt2e/language-config branch October 8, 2024 04:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: separate out language logic from moduleconfig #3016

feat: separate out language logic from moduleconfig #3016

matt2e commented Oct 7, 2024 •

edited

Loading

matt2e Oct 7, 2024 •

edited

Loading

alecthomas Oct 7, 2024

matt2e Oct 7, 2024

matt2e Oct 7, 2024

matt2e Oct 7, 2024

matt2e Oct 7, 2024

matt2e Oct 7, 2024

matt2e Oct 7, 2024 •

edited

Loading

matt2e Oct 7, 2024

feat: separate out language logic from moduleconfig #3016

feat: separate out language logic from moduleconfig #3016

Conversation

matt2e commented Oct 7, 2024 • edited Loading

Why

Creating a ModuleConfig

Other changes

matt2e Oct 7, 2024 • edited Loading

Choose a reason for hiding this comment

alecthomas Oct 7, 2024

Choose a reason for hiding this comment

matt2e Oct 7, 2024

Choose a reason for hiding this comment

matt2e Oct 7, 2024

Choose a reason for hiding this comment

matt2e Oct 7, 2024

Choose a reason for hiding this comment

matt2e Oct 7, 2024

Choose a reason for hiding this comment

matt2e Oct 7, 2024

Choose a reason for hiding this comment

matt2e Oct 7, 2024 • edited Loading

Choose a reason for hiding this comment

matt2e Oct 7, 2024

Choose a reason for hiding this comment

matt2e commented Oct 7, 2024 •

edited

Loading

matt2e Oct 7, 2024 •

edited

Loading

matt2e Oct 7, 2024 •

edited

Loading