-
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
[KS-87] Generic workflow engine #12461
Conversation
* Add hardcoded YAML workflow and parsing * Execute an individual step * Execute an individual step
e0591a0
to
bbdac51
Compare
I see that you haven't updated any README files. Would it make sense to do so? |
bbdac51
to
e5f277e
Compare
e5f277e
to
3fb38d8
Compare
3fb38d8
to
da4b39b
Compare
da4b39b
to
f1f16f0
Compare
f1f16f0
to
d0b2137
Compare
d0b2137
to
8d8ae47
Compare
8d8ae47
to
e90f8a8
Compare
e90f8a8
to
6a33827
Compare
6a33827
to
539e083
Compare
539e083
to
d17b0c2
Compare
d17b0c2
to
4c2a220
Compare
4c2a220
to
b7e188b
Compare
34bf71c
to
7e75cd1
Compare
7e75cd1
to
2b14b85
Compare
2b14b85
to
d9b6032
Compare
d9b6032
to
6f096ba
Compare
…ments' into ks-87-workflow-engine-scheduler
617d3cf
to
54bb815
Compare
newWorkerTimeout time.Duration | ||
|
||
// Used for testing to wait for an execution to complete | ||
xxxExecutionFinished chan string |
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.
Not a huge fan of these kind of hooks. Can we maybe use a mock target in tests and wait for it to be called?
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.
Same... I initially used a mock target but that leads to a race since the engine will do some processing (like updating internal state) after the target has been called. It also won't notify you if the workflow has errored for some reason.
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.
going to let someone more experienced like @bolekk to approve, LGTM though!
// 2. Registers each step's capability to this workflow | ||
// 3. Registers for trigger events now that all capabilities are resolved | ||
// | ||
// Steps 1 and 2 are retried every 5 seconds until successful. |
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.
Do we want a retry limit?
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.
Hmmm... probably not -- I think this should be a temporary thing, and if we're not able to initialize the workflow we should keep trying since the NOP has chosen to run the job.
return nil | ||
} | ||
|
||
// If the capability is already cached, that means we've already registered it |
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.
// If the capability is already cached, that means we've already registered it | |
// If the capability instance already exists, that means we've already registered it |
Quality Gate passedIssues Measures |
No description provided.