Skip to content

Conversation

shubham1172
Copy link
Member

Signed-off-by: Shubham Sharma shubhash@microsoft.com

Description

Adds a few more features to the dapr bot

  1. Cron job to mark issues and PR stale
  2. /ok-to-e2e-test to retrigger end to end tests
  3. /ping-author to add special label needs-author-feedback
  4. Replacing needs-author-feedback with needs-team-attention when author replies after (3)

Issue reference

Please reference the issue this PR will close: #208

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

  • (NA) Code compiles correctly
  • (NA) Created/updated tests
  • (NA) Extended the documentation

Signed-off-by: Shubham Sharma <shubhash@microsoft.com>
@shubham1172 shubham1172 requested review from a team as code owners March 10, 2022 09:46
@codecov
Copy link

codecov bot commented Mar 10, 2022

Codecov Report

Merging #210 (f8ffe77) into master (4b8478e) will increase coverage by 5.39%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #210      +/-   ##
==========================================
+ Coverage   25.30%   30.70%   +5.39%     
==========================================
  Files          64       70       +6     
  Lines        5402     5472      +70     
  Branches      172      172              
==========================================
+ Hits         1367     1680     +313     
+ Misses       4031     3758     -273     
- Partials        4       34      +30     
Impacted Files Coverage Δ
src/actors/ActorId.ts 100.00% <ø> (+66.66%) ⬆️
src/actors/client/ActorClient/ActorClient.ts 72.22% <ø> (+38.88%) ⬆️
src/actors/client/ActorClient/ActorClientGRPC.ts 3.53% <ø> (ø)
src/actors/client/ActorClient/ActorClientHTTP.ts 44.44% <ø> (+38.88%) ⬆️
src/actors/client/ActorProxyBuilder.ts 100.00% <ø> (+88.23%) ⬆️
src/actors/runtime/AbstractActor.ts 74.28% <ø> (+62.85%) ⬆️
src/actors/runtime/ActorManager.ts 70.73% <ø> (+58.53%) ⬆️
src/actors/runtime/ActorReminderData.ts 52.94% <ø> (+41.17%) ⬆️
src/actors/runtime/ActorRuntime.ts 72.97% <ø> (+59.45%) ⬆️
src/actors/runtime/ActorRuntimeConfig.ts 73.33% <ø> (+60.00%) ⬆️
... and 93 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e56f31d...f8ffe77. Read the comment docs.

@shubham1172 shubham1172 changed the title Enhance Dapr Bot [WIP] Enhance Dapr Bot Mar 10, 2022
Copy link

@tanvigour tanvigour left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should also have something for better stale management of issues. For example, if there's no activity for say 3 days, ping a maintainer?

@shubham1172
Copy link
Member Author

@tanvigour I can think of two scenarios here.

  1. User opens an issue saying "Help with XYZ" and when we ask for more context, they ghost us. In this case, the action item is on them, and stale bot will simply mark stale/close it after threshold days.
  2. There is a feature request/bug which requires action item from a contributor/maintainer. In this case, the stale bot should not close the issue. This can be prevented by adding special labels to the issue (mentioned in the workflow).

There is another aspect to it. Maintainers and contributors are responsible for regularly triaging the issues. They also have their GitHub notifications already hot, so any ping we might add will add to the noise. Plus, it is possible that the action item is already on a single maintainer, so in that case, how do we ping only the person who's responsible? IMO, we can skip this feature for now and manually keep an eye on issues. Case (1) above is the purpose of the stale bot, and Case (2) can be avoided by regular triaging of the repository (the maintainer can simply add the label triaged/resolved).

Also, this repository does not see a huge flux of issues. The current count is also very less. Chances are that each issue is already under a contributor's radar.

Please feel free to share your thoughts - @XavierGeerinck @amulyavarote

@XavierGeerinck
Copy link
Contributor

I agree with you @shubham1172 I would however tag wise look at the needs-author-feedback and needs-team-attention labels instead of triaged/resolved? Seeing that those are the "ping/pong" mechanism and only ping us whenever we know there is an action at our side. It will also prevent the stale bot removing those issues

tanvigour
tanvigour previously approved these changes Mar 21, 2022
amulyavarote
amulyavarote previously approved these changes Mar 21, 2022
Signed-off-by: Shubham Sharma <shubhash@microsoft.com>
@shubham1172 shubham1172 dismissed stale reviews from amulyavarote and tanvigour via c917646 March 28, 2022 10:44
Signed-off-by: Shubham Sharma <shubhash@microsoft.com>
Signed-off-by: Shubham Sharma <shubhash@microsoft.com>
@shubham1172 shubham1172 changed the title [WIP] Enhance Dapr Bot Enhance Dapr Bot Mar 28, 2022
@shubham1172
Copy link
Member Author

@XavierGeerinck

I agree with you @shubham1172 I would however tag wise look at the needs-author-feedback and needs-team-attention labels instead of triaged/resolved? Seeing that those are the "ping/pong" mechanism and only ping us whenever we know there is an action at our side. It will also prevent the stale bot removing those issues

When an issue is tagged with needs-author-feedback, we should still be able to mark them stale, right? Say someone posted an issue where they are facing XYZ issues and we ask for more information, adding the tag needs-author-feedback, but they don't get back in 60 days.

Copy link
Contributor

@XavierGeerinck XavierGeerinck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overal "fine" some minor style changes.

Big question I have: Wouldn't it be easier and more readable if we process by comment body and respective functions?

E.g. (pseudo wise)

switch (commentBody) {
    case "/assign":
        executeAssign();
        break;
    case "/other":
        executeOther();
        break;
}

Signed-off-by: Shubham Sharma <shubhash@microsoft.com>
Signed-off-by: Shubham Sharma <shubhash@microsoft.com>
@shubham1172
Copy link
Member Author

Overal "fine" some minor style changes.

Big question I have: Wouldn't it be easier and more readable if we process by comment body and respective functions?

E.g. (pseudo wise)

switch (commentBody) {
    case "/assign":
        executeAssign();
        break;
    case "/other":
        executeOther();
        break;
}

It was mostly a port of dapr/dapr earlier. I have refactored it - but it comes with some boilerplates. This is more readable for sure but. Let me know if this works!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve Dapr-Bot
4 participants