Support flexible folder structures #542

jacobgreenleaf · 2019-01-09T02:35:21Z

An assumption of the base module system was that the module classes (clients, endpoints, etc.) would have a single folder structure that would be extended (but not modified) by consumer applications. Specifically, the code generation system is given a base directory (baseDirectory) and it assumes the built-in modules would be located in exactly the subdirectory of that base directory (e.g. if baseDirectory = "examples/example-gateway" then endpoints will always be located in examples/example-gateway/endpoints). That assumption is encoded in the Directories property, which enumerates all the places where Zanzibar will look for module instances.

This poses a problem for supporting more flexible directory structures, such as keeping clients at examples/example-gateway/clients but allowing instances of endpoints to exist in examples/example-gateway/apps/foo/endpoints; applications could not customize the place where Zanzibar would look for module instances.

This patch changes the module search logic to be more flexible. Instead of the assumption that the place where Zanzibar configuration files like clients will be found is known ahead of time, we instead assume that we will be provided a list of globbing patterns to enumerate all the configuration files that exist:

moduleSearchPaths:
- clients/*
- middlewares/*
- endpoints/*
- endpoints/tchannel/*
- services/*
- app/*/clients/*
- app/*/endpoints/*
- app/*/services/*

The list will be provided in the build.yaml file which is provided by the application and can be customized without modifying Zanzibar. Zanzibar will then look for every instance type it is aware of in every folder. For example, you could have:

clients/
  foo/
    client-config.yaml
  bar/
    endpoint-config.yaml

There's nothing special about the relationship between the module classes and their paths.

Dependency names as stored in the map[string][]*ModuleInstance "tree" are now first extracted from the path name:

Path	Qualified Instance Name
`clients/foo/client-config.yaml`	`foo`
`nested/clients/foo/client-config.yaml`	`nested/foo`
`nested/bar/foo/baz/foo-config.yaml`	`nested/bar/baz`
`app/demo/services/xyz/service-config.yaml`	`app/demo/xyz`

As a convenience, the second to last part of the qualified name is removed (clients/foo is transformed to foo). To depend on a so-called nested dependency, you would use the qualified instance name in the dependency list:

dependencies:
  endpoint:
  - app/demo/abc
  - health

TODO:

Create an example case for fully qualified dependency name
Verify nested service starts up
Fix mock generation
Fix lint
~~[ ] (?) Remove name field from *-config.yaml since it's now implied by the path~~

ChuntaoLu

lgtm overall, a few minor things commented

ChuntaoLu · 2019-01-09T22:35:26Z

codegen/module.go

@@ -134,26 +129,10 @@ func (system *ModuleSystem) validateClassDir(class *ModuleClass, dir string) err
 // RegisterClassDir adds the given dir to the directories of the module class with given className.
 // This method allows projects built on zanzibar to have arbitrary directories to host module class
 // configs, therefore is mainly intended for external use.
+//
+// Deprecated: Module search paths deprecates this function.


Since we are on 0.1 release, shouldn't we just remove this method? This way we don't need do cleanup work in the future.

ChuntaoLu · 2019-01-09T22:37:13Z

codegen/module.go

@@ -218,6 +197,8 @@ func (system *ModuleSystem) populateResolvedDependencies(
 			}

 			if dependencyInstance == nil {
+				fmt.Printf("known module instances: %+v", resolvedModules)


debug leftover?

ChuntaoLu · 2019-01-09T23:05:10Z

codegen/module.go

+					// matched by a longer glob pattern like "endpoints/tchannel/*", so merely print a warning here
+					// rather than failing to generate. It will fail at a later step if no globbing pattern matches
+					// the wanted instance.
+					fmt.Printf("warn: globbing pattern %q: %s\n", moduleDirectoryGlob, instanceErr.Error())


This warning message seems unnecessary to me, it warns for each dir that does not contain a module config file, which could become quite a clutter when you have many modules nested.

ChuntaoLu · 2019-01-09T23:06:19Z

codegen/module.go

+		if strings.HasSuffix(instanceFile.Name(), yamlConfigSuffix) {
+			className = strings.TrimSuffix(instanceFile.Name(), yamlConfigSuffix)
+			yamlFileName = instanceFile.Name()
+			// TODO(jacobg): Is this necessary?


I don't think it's necessary, should just delete it to avoid future confusion.

ChuntaoLu · 2019-01-09T23:07:50Z

codegen/module.go

+	NamePlural string
+	ClassType  moduleClassType
+
+	// Deprecated: Hard coded list of directory names is replaced with dynamic module search path in build.yaml.


same as earlier, deletion instead of deprecation.

ChuntaoLu · 2019-01-09T23:16:44Z

examples/example-gateway/build.yaml

+moduleSearchPaths:
+- clients/*
+- middlewares/*
+- endpoints/*


Since this is glob, I would not be surprised to see patterns like "endpoints/*/**" in the yaml file, can we have such a case to make sure things work as intended?

** is not supported, see https://golang.org/pkg/path/filepath/#Match

okay, does that mean endpoint/* does not match endpoint/foo/bar/endpoint-config.yaml?

Yes, you'd need to write endpoint/*/* to capture that, we might want a more flexible globbing pattern library but I just used the standard library one for now.

Okay, got it, this is definitely something worth to document.

coveralls · 2019-02-06T22:20:29Z

Coverage increased (+7.3%) to 68.288% when pulling 9001235 on jg-appfolders into c7b8c7f on master.

jacobgreenleaf requested review from ChuntaoLu and herainman January 9, 2019 03:00

jacobgreenleaf force-pushed the jg-appfolders branch 3 times, most recently from 19cd42d to 439cf00 Compare January 9, 2019 21:27

ChuntaoLu reviewed Jan 9, 2019

View reviewed changes

jacobgreenleaf force-pushed the jg-appfolders branch from 439cf00 to 24f46f4 Compare January 31, 2019 02:21

jacobgreenleaf changed the title ~~WIP: Support flexible folder structures~~ Support flexible folder structures Feb 1, 2019

jacobgreenleaf force-pushed the jg-appfolders branch from 683ddd5 to 5402cdc Compare February 6, 2019 21:17

Support flexible folder structures

9001235

jacobgreenleaf force-pushed the jg-appfolders branch from 4120638 to 9001235 Compare February 6, 2019 22:08

ChuntaoLu approved these changes Feb 6, 2019

View reviewed changes

jacobgreenleaf merged commit 7e3da0b into master Feb 6, 2019

jacobgreenleaf deleted the jg-appfolders branch February 6, 2019 22:56

jacobgreenleaf restored the jg-appfolders branch February 6, 2019 22:56

ChuntaoLu deleted the jg-appfolders branch July 8, 2020 17:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support flexible folder structures #542

Support flexible folder structures #542

jacobgreenleaf commented Jan 9, 2019 •

edited

Loading

ChuntaoLu left a comment

ChuntaoLu Jan 9, 2019

ChuntaoLu Jan 9, 2019

ChuntaoLu Jan 9, 2019

ChuntaoLu Jan 9, 2019

ChuntaoLu Jan 9, 2019

ChuntaoLu Jan 9, 2019

jacobgreenleaf Feb 6, 2019

ChuntaoLu Feb 6, 2019

jacobgreenleaf Feb 6, 2019

ChuntaoLu Feb 6, 2019

coveralls commented Feb 6, 2019

Support flexible folder structures #542

Support flexible folder structures #542

Conversation

jacobgreenleaf commented Jan 9, 2019 • edited Loading

ChuntaoLu 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

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

Choose a reason for hiding this comment

coveralls commented Feb 6, 2019

jacobgreenleaf commented Jan 9, 2019 •

edited

Loading