[CLD-16]: feat(operation): introduce operation component #16756
Conversation
graham-chainlink
force-pushed
the
ggoh/CLD-16/operation-initial-component
branch
2 times, most recently
from
dfd8495 to
e674a92
Compare
deployment/operation.go
Outdated
| // OperationContext is the context passed to the OperationHandler. | ||
| // It contains the logger. | ||
| // Use NewOperationContext to create a new OperationContext. | ||
| type OperationContext struct { | ||
| Logger logger.Logger | ||
| } |
There was a problem hiding this comment.
The naming of this could be confusing as it is not really a context.Context. Should we passing a real context down into the operation so that it can be cancelled as well?
There was a problem hiding this comment.
after some thinking, i am going to rename this to OperationEnv similar to how we have deployment.environment in changesets.
Should we passing a real context down into the operation so that it can be cancelled as well?
yeh interesting, i was originally thinking we dont need to since deployment.environment already has it and execution logic can access it directly, but depending how user wants to structure their operation, it may or may not have direct access to deployment.environment, eg in Rodrigo PR, operation is created in a new location.
There was a problem hiding this comment.
I think env could get confusing as well. How do you feel about OperationScope?
There was a problem hiding this comment.
Hmm i thought having it as OperationEnv would be more consistent with deployment.environment in changesets as we use it similarly to pass logger and context.
is OperationScope really better? I feel like it is still vague ish but i also cant think of a better name.
There was a problem hiding this comment.
I think OperationScope is only better because it doesn't reuse the environment name which is already an overloaded term in CLD
There was a problem hiding this comment.
but i think with this , cyclic dependency will be an issue , as deployment package will call operation.
Now i am thinking should we move all those framework code into a new package which will help in the future work to migrate the framework into a new repo or we have to move all the operation stuff into the same deployment package.
There was a problem hiding this comment.
After some discussion with James, we think that maybe passing the whole deployment.Environment in OperationEnvironment feels like passing the whole kitchen sink, an Operation can have Dependencies , and if user wants something from the environment like client, user could pass it in the Deps per Rodrigo PR
What your thought @cgruber ?
There was a problem hiding this comment.
so treating it as an OpEnvironment which contains a deployment.Environment seems reasonable.
@cgruber @graham-chainlink This would break the entire idea behind Operations. They should explicitly type the requested Input and Dependencies, if we pass the entire deployment.Environment developers wouldn't need to require/inject any dependency, and operations would become unpredictable, hardly composable and testable
There was a problem hiding this comment.
alright so for naming, we have a few choices
- operations.Environment ( per this PR)
- operations.Scope (suggested by James)
operations.Session- operations.Context
Thoughts?
There was a problem hiding this comment.
Alright i have settled on calling it operations.Bundle
graham-chainlink
force-pushed
the
ggoh/CLD-16/operation-initial-component
branch
from
e674a92 to
032e460
Compare
graham-chainlink
force-pushed
the
ggoh/CLD-16/operation-initial-component
branch
2 times, most recently
from
f1dbb7a to
4e603e3
Compare
graham-chainlink
force-pushed
the
ggoh/CLD-16/operation-initial-component
branch
from
4e603e3 to
add3754
Compare
deployment/operations/operation.go
Outdated
| } | ||
|
|
||
| // execute runs the operation by calling the OperationHandler. | ||
| func (o *Operation[I, O, D]) execute(e OperationEnv, deps D, input I) (output O, err error) { |
There was a problem hiding this comment.
this method is private as only execution client (introduce in future PR) should call this method , our users should not be calling this directly
graham-chainlink
requested review from
winder,
asoliman92,
matYang,
makramkd,
dimkouv,
mateusz-sekara and
0xnogo
graham-chainlink
force-pushed
the
ggoh/CLD-16/operation-initial-component
branch
from
add3754 to
e36cd44
Compare
deployment/operations/operation.go
Outdated
| // Definition is the metadata for a sequence or an operation. | ||
| type Definition struct { | ||
| ID string | ||
| Version semver.Version |
There was a problem hiding this comment.
Maybe add a descriptio of how semver will be used, if it's purely documentary, or if it has any semantic impact on execution.
There was a problem hiding this comment.
Updated to mention version serves as part of the composite key for operation
deployment/operation.go
Outdated
| // OperationContext is the context passed to the OperationHandler. | ||
| // It contains the logger. | ||
| // Use NewOperationContext to create a new OperationContext. | ||
| type OperationContext struct { | ||
| Logger logger.Logger | ||
| } |
There was a problem hiding this comment.
To me it functions in the same ecological niche as the environment.
Also, we probably don't want operations' functions accessing the environment from the lexical scope capture (for testability, among other reasons, we want these to be as pure-functions as we can). So we probably want whatever we call this object to expose the deployment.Environment anyway, so treating it as an OpEnvironment which contains a deployment.Environment seems reasonable.
graham-chainlink
force-pushed
the
ggoh/CLD-16/operation-initial-component
branch
from
e36cd44 to
d261133
Compare
graham-chainlink
force-pushed
the
ggoh/CLD-16/operation-initial-component
branch
9 times, most recently
from
dac53b1 to
38a772b
Compare
|
I still don't like reusing the word I've given it an approval but we should wait for @cgruber before merging |
Thanks, yeah , i am thinking of a better word to use :D |
Introducing the first building block for the operations API, this commit introduces the Operation component. Currently, it does not cover Report, retry policy , those will be in future PRs JIRA: https://smartcontract-it.atlassian.net/browse/CLD-16
graham-chainlink
dismissed stale reviews from RodrigoAD and jkongie
via
0540617
graham-chainlink
force-pushed
the
ggoh/CLD-16/operation-initial-component
branch
from
38a772b to
0540617
Compare
|
| assert.Equal(t, "sum", entry.ContextMap()["id"]) | ||
| assert.Equal(t, version.String(), entry.ContextMap()["version"]) | ||
| assert.Equal(t, description, entry.ContextMap()["description"]) | ||
| } |
There was a problem hiding this comment.
can we also add test where handler uses empty input ?
There was a problem hiding this comment.
is there a particular scenario you are thinking? The empty input test doesnt cover any extra logic or scenario.
There was a problem hiding this comment.
I just meant to cover it e2e. so to addition smth like
func Test_Operation_WithEmptyInput(t *testing.T) {
t.Parallel()
handler := func(b Bundle, deps OpDeps, _ EmptyInput) (string, error) {
return "ok", nil
}
op := NewOperation("noop", semver.MustParse("1.0.0"), "no-op", handler)
out, err := op.execute(NewBundle(context.Background, logger.Test(t)), OpDeps{}, EmptyInput{})
require.NoError(t, err)
assert.Equal(t, "ok", out)
}
its nit
There was a problem hiding this comment.
okay, i will add it in my next PR
There was a problem hiding this comment.
added it here #16801
Introducing the first building block for the operations API, this commit introduces the Operation component.
Ported from Rodrigo spike PR, but updated with small changes like factory methods and naming changes.
Currently, it does not include Report, retry policy , those will be in future PRs. One small PR at a time for feedback to incorporate any improvements.
JIRA: https://smartcontract-it.atlassian.net/browse/CLD-16