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

refactor: centralise config/secrets/dbs/mocks into ModuleContext #1395

Merged
merged 1 commit into from
May 3, 2024

Conversation

matt2e
Copy link
Collaborator

@matt2e matt2e commented May 3, 2024

  • Reduces reliance on configuration package in go-runtime
  • Removed CallOverrider protocol:
    • call.go now asks ModuleContext for any custom behaviour as required by the test set up. This includes verb mocks, whether to allow calling the verb directly, or custom errors in testing environments

@matt2e matt2e requested a review from a team as a code owner May 3, 2024 01:08
@matt2e matt2e requested review from worstell and removed request for a team May 3, 2024 01:08
@alecthomas alecthomas mentioned this pull request May 3, 2024
@matt2e matt2e marked this pull request as draft May 3, 2024 01:09
@@ -60,6 +55,64 @@ func TestManager(t *testing.T) {
})
}

// TestMapPriority checks that module specific configs beat global configs when flattening for a module
func TestMapPriority(t *testing.T) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

moved from module_context_test.go

@@ -1,8 +0,0 @@
[global]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed because config/secret behavior is no longer tightly tied to project files

// This is useful for testing and development, where the environment is used to provide
// configurations, secrets and DSNs. The context is built from a combination of
// the ftl-project.toml file and (for now) environment variables.
func FromEnvironment(ctx context.Context, module string, isTesting bool) (*ModuleContext, error) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

moved from module_context.go

@matt2e matt2e force-pushed the matt2e/go-runtime-no-config branch from 247e043 to 1b31a1d Compare May 3, 2024 01:14
@matt2e matt2e marked this pull request as ready for review May 3, 2024 01:21
// configurations, secrets and DSNs. The context is built from a combination of
// the ftl-project.toml file and (for now) environment variables.
func FromEnvironment(ctx context.Context, module string, isTesting bool) (*ModuleContext, error) {
// TODO: split this func into separate purposes: explicitly reading a particular project file, and reading DSNs from environment
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Related to this issue: #1391

@matt2e matt2e force-pushed the matt2e/go-runtime-no-config branch from 1b31a1d to 658ea91 Compare May 3, 2024 01:25
if mock, ok := m.mockVerbs[ref]; ok {
return MockBehavior{Mock: mock}, nil
}
// TODO: add logic here for when to do direct behavior
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will be done in this PR after it rebases on this one: #1373

@matt2e matt2e merged commit 5b621ea into main May 3, 2024
10 checks passed
@matt2e matt2e deleted the matt2e/go-runtime-no-config branch May 3, 2024 01:34
)

type Ref struct {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we use a schema.RefKey here? We're proliferating Refs for no real reason I think, so I'm trying to consolidate them. schema.Ref has a method to retrieve a RefKey.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will do 👍

return moduleCtx
}

func FromContext(ctx context.Context) *ModuleContext {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Comments.

Please add useful comments to all code going into go-runtime.

}

// SetSecret sets a secret value for the module.
func (m *ModuleContext) SetSecret(name string, value any) error {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we nede setters? Can we not make the ModuleContext immutable?

If we don't, it will need locks because it could be accessed from multiple threads, but I'd vastly prefer it to be immutable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will make another issue for that

return resp, fmt.Errorf("%s: %w", callee, err)
}
switch behavior := behavior.(type) {
case modulecontext.MockBehavior:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of a switch in the call function with logic in each case, move the logic into the respective VerbBehaviour implementations. A general pattern to watch for is that if significant logic around a "switch" is occurring outside the thing being switched on it might be time to encapsulate that logic into a method.

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.

3 participants