-
Notifications
You must be signed in to change notification settings - Fork 1.9k
[CRE-533] Update Fake Consensus to support mode-switch and report-gen #18456
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
Conversation
|
👋 @bolekk, thanks for creating this pull request! To help reviewers, please consider creating future PRs as drafts first. This allows you to self-review and make any final changes before notifying the team. Once you're ready, you can mark it as "Ready for review" to request feedback. Thanks! |
c964643 to
5c7eca2
Compare
5c7eca2 to
de3f123
Compare
|
| } | ||
| caps = append(caps, streamsTrigger) | ||
|
|
||
| httpAction := fakes.NewDirectHTTPAction(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.
@justinkaseman , @akhilchainani please confirm that it's OK to get those back. I'm not sure why you removed them in an earlier PR.
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.
yeah add them back, idk how they got removed. Probably by accident, sorry about that
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.
thanks!
| var _ commonCap.ExecutableCapability = (*DirectHTTPAction)(nil) | ||
|
|
||
| const HTTPActionID = "http-action@1.0.0" | ||
| const HTTPActionID = "http-actions@0.1.0" |
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.
is this the new id used in the sdk?
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.
It seems like. That's what a freshly-built workflow asks for...
| } | ||
| caps = append(caps, streamsTrigger) | ||
|
|
||
| httpAction := fakes.NewDirectHTTPAction(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.
yeah add them back, idk how they got removed. Probably by accident, sorry about that
| return response, nil | ||
| } | ||
|
|
||
| func (fh *DirectHTTPAction) Start(ctx context.Context) error { |
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.
How do we start caps now without 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.
This was actually a bug - overriding Start() and Close() from the underlying Service object. @justinkaseman 's test was failing. Start() and Close() are still there and if we want to customize they need to be named differently (e.g. lowercase)




No description provided.