KS-15 Add a capabilities library to the core node #11811
Conversation
|
I see that you haven't updated any CHANGELOG files. Would it make sense to do so? |
cedric-cordenier
had a problem deploying
to
sdlc
GitHub Actions
Failure
cedric-cordenier
force-pushed
the
KS-15-add-a-capabilities-library-to-the-core-node
branch
from
8ff53ea to
7528c5f
Compare
cedric-cordenier
had a problem deploying
to
sdlc
GitHub Actions
Failure
cedric-cordenier
force-pushed
the
KS-15-add-a-capabilities-library-to-the-core-node
branch
from
7528c5f to
7c66aba
Compare
cedric-cordenier
had a problem deploying
to
sdlc
GitHub Actions
Failure
cedric-cordenier
force-pushed
the
KS-15-add-a-capabilities-library-to-the-core-node
branch
from
7c66aba to
e60d1d9
Compare
cedric-cordenier
had a problem deploying
to
sdlc
GitHub Actions
Failure
cedric-cordenier
force-pushed
the
KS-15-add-a-capabilities-library-to-the-core-node
branch
from
e60d1d9 to
cabc0f6
Compare
cedric-cordenier
had a problem deploying
to
sdlc
GitHub Actions
Failure
cedric-cordenier
force-pushed
the
KS-15-add-a-capabilities-library-to-the-core-node
branch
from
cabc0f6 to
cff4ea5
Compare
cedric-cordenier
had a problem deploying
to
sdlc
GitHub Actions
Failure
cedric-cordenier
force-pushed
the
KS-15-add-a-capabilities-library-to-the-core-node
branch
from
cff4ea5 to
1ed979b
Compare
cedric-cordenier
had a problem deploying
to
sdlc
GitHub Actions
Failure
cedric-cordenier
force-pushed
the
KS-15-add-a-capabilities-library-to-the-core-node
branch
from
1ed979b to
f636f06
Compare
cedric-cordenier
had a problem deploying
to
sdlc
GitHub Actions
Failure
cedric-cordenier
had a problem deploying
to
sdlc
GitHub Actions
Failure
DeividasK
force-pushed
the
KS-15-add-a-capabilities-library-to-the-core-node
branch
from
3e413bd to
2291339
Compare
core/capabilities/capability.go
Outdated
| // when the context is cancelled. When a request has been completed the capability | ||
| // is also expected to close the callback channel. | ||
| // Request specific configuration is passed in via the inputs parameter. | ||
| Execute(ctx context.Context, callback chan values.Value, inputs values.Map) error |
There was a problem hiding this comment.
are we adding the name field?
| } | ||
|
|
||
| // CapabilityInfo is a struct for the info of a capability. | ||
| type CapabilityInfo struct { |
There was a problem hiding this comment.
Where does this return what the strong type is?
There was a problem hiding this comment.
Hmmmm -- what do you mean by "strong type"?
There was a problem hiding this comment.
@nolag are you talking about CapabilityType?
| } | ||
|
|
||
| // Info returns the info of the capability. | ||
| func (c CapabilityInfo) Info() CapabilityInfo { |
There was a problem hiding this comment.
May want to return map[string]CapabilityInfo so we can add name to execute.
There was a problem hiding this comment.
CapabilityInfo includes ID, isn't that enough?
There was a problem hiding this comment.
This was a separate conversation that is resolved now - we've parked this for now and @cedric-cordenier will write an exploration doc on whether we want to pass in a name field or not.
core/capabilities/capability.go
Outdated
| // when the context is cancelled. When a request has been completed the capability | ||
| // is also expected to close the callback channel. | ||
| // Request specific configuration is passed in via the inputs parameter. | ||
| Execute(ctx context.Context, callback chan values.Value, inputs values.Map) error |
There was a problem hiding this comment.
I left this question in the CRON doc: if Execute() registers a trigger, how do we de-register it when a workflow is deleted?
There was a problem hiding this comment.
This is the responsibility of a workflow engine. There are 2 steps to capabilities:
- A capability is registered by a NOP.
- A capability is
started when a workflow requiring it is received.
(I can already see the comment on line 47 being wrong 😅)
Similarly, when a workflow requiring a capability is removed, the capability should be stoped. @cedric-cordenier we'll need to add more details here.
There was a problem hiding this comment.
OK, if start/stop is per workflow then it makes more sense.
core/capabilities/capability.go
Outdated
| // Capability must respect context.Done and cleanup any request specific resources | ||
| // when the context is cancelled. When a request has been completed the capability | ||
| // is also expected to close the callback channel. | ||
| // Request specific configuration is passed in via the inputs parameter. |
There was a problem hiding this comment.
Outdated comment? I do not see an inputs parameter
cedric-cordenier
force-pushed
the
KS-15-add-a-capabilities-library-to-the-core-node
branch
from
974d6f2 to
7b8ac45
Compare
DeividasK
changed the title
| panic("unknown capability type") | ||
| } | ||
|
|
||
| // IsValid checks if the capability type is valid. |
There was a problem hiding this comment.
nit: some comments feel excessive in this file, code is self-explanatory
There was a problem hiding this comment.
huh? is golint forcing us to have comments everywhere?
| } | ||
|
|
||
| // CapabilityInfo is a struct for the info of a capability. | ||
| type CapabilityInfo struct { |
There was a problem hiding this comment.
@nolag are you talking about CapabilityType?
| } | ||
|
|
||
| // Info returns the info of the capability. | ||
| func (c CapabilityInfo) Info() CapabilityInfo { |
There was a problem hiding this comment.
CapabilityInfo includes ID, isn't that enough?
| "v1.0.0", | ||
| ) | ||
|
|
||
| // OnDemandTrigger is an example on-demand trigger. |
There was a problem hiding this comment.
Can we explain that this trigger supports sending ad-hoc events to registered listeners?
| } | ||
|
|
||
| // Execute executes the on-demand trigger. | ||
| func (o *OnDemandTrigger) SendEvent(ctx context.Context, wid string, event capabilities.CapabilityResponse) error { |
There was a problem hiding this comment.
I'm wondering if this is a good example. Should a trigger ever support targeting a particular workflow ID? It seems like listeners should be transparent to triggers. Let me know if I'm missing an obvious use case.
|
SonarQube Quality Gate
|
Paired with @cedric-cordenier (and by paired, I mean that he did most of the work 😅).
The initial set of interfaces. This will be iterated in follow-up PRs, but we want to create a reference point to parallelize capability development.