-
Notifications
You must be signed in to change notification settings - Fork 917
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
work suspension: emit event for work dispatch status #5332
Conversation
/hold until #5317 is merged |
Hi @a7i, we can continue this pr. Apart from the documentation, this should be the last pr. Can you think of any other tasks that need to be done? |
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #5332 +/- ##
==========================================
- Coverage 28.53% 28.51% -0.03%
==========================================
Files 632 632
Lines 43856 43862 +6
==========================================
- Hits 12515 12506 -9
- Misses 30443 30451 +8
- Partials 898 905 +7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: Amir Alavi <[email protected]> fix lint fix lint
I would like to add a bit more e2e tests (will try to add more unit tests but requires some setup). I can work on the documentation soon and e2e soon. Also, I don't know how trivial this would be, but ideally we can have a metric for work status (with tag value for dispatching vs suspend dispatching), unless it's already there. |
Just rebased the PR, I ran golangci-lint locally and it applied a couple of minor fixes. Let me know if you want that reverted. |
Thanks @a7i /cc @chaunceyjiang |
How do you think? @chaunceyjiang @RainbowMango |
It is concerning if we are talking about introducing a metric that tells |
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.
/lgtm
/assign @XiShanYongYe-Chang
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~
/lgtm
/hold cancel |
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~
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: chaunceyjiang The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What type of PR is this?
/kind feature
What this PR does / why we need it:
As requested by @chaunceyjiang here
Which issue(s) this PR fixes:
Part of #5217
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
It's definitely a noisy one (from e2e):