-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Mock streams trigger #14078
Mock streams trigger #14078
Conversation
I see you updated files related to
|
@@ -206,6 +207,13 @@ func NewApplication(opts ApplicationOpts) (Application, error) { | |||
opts.CapabilitiesRegistry = capabilities.NewRegistry(globalLogger) | |||
} | |||
|
|||
// Use a recurring trigger with mock data for testing purposes | |||
// TODO: proper component shutdown via srvcs() | |||
_, err := capStreams.RegisterMockTrigger(globalLogger, opts.CapabilitiesRegistry) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@archseer Really not wild about this: can we package up the mock as a standard capability instead? That way there won't be any code hanging around the core node to support this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively, trigger capabilities should provide a mocked version of themselves (same output types, different implementation).
@@ -258,6 +266,9 @@ func NewApplication(opts ApplicationOpts) (Application, error) { | |||
|
|||
srvcs = append(srvcs, wfLauncher, registrySyncer) | |||
} | |||
} else { | |||
// If registry syncer is not set up we use a dummy local registry so that local capabilities can still be used |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@archseer this shouldn't be fallback behaviour: this should only be set explicitly if a specific config flag is set (eg. if the registryAddress == "MOCK_REGISTRY_ADDRESS" or similar
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not though? If no registry is set the node is in a broken state where local capabilities won't work. By providing a mock we're at least able to use workflows
Quality Gate passedIssues Measures |
func NewMockDataProducer(trigger *MockTriggerService, meta datastreams.SignersMetadata, signers []*ecdsa.PrivateKey, feedIDs []string, lggr logger.Logger) *mockDataProducer { | ||
return &mockDataProducer{ | ||
trigger: trigger, | ||
closeCh: make(chan struct{}), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
closeCh
doesn't seem used
} | ||
|
||
// Only start the producer once a workflow is registered | ||
o.producer = NewMockDataProducer(o, o.meta, o.signers, config.FeedIDs, o.lggr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we close the old producer before starting a new one, or just start it once on first registration?
Closing for now, refactoring this |
Makes it easier to develop and test workflows by providing a data source compatible with streams trigger, but mocked. It also stubs out the local capability registry if an external one isn't defined, this allows us to test workflows without having to have a capability registry up and running.