alexellis / derek

Reduce maintainer fatigue by automating GitHub
https://github.com/alexellis/derek/blob/master/USER_GUIDE.md
MIT License
806 stars 72 forks source link

Proposal: support multiple commands in one comment #110

Closed kolyshkin closed 5 years ago

kolyshkin commented 5 years ago

After reading the docs I got the impression I can specify multiple commands, so I added the following comment (on https://github.com/moby/moby/pull/38634):

d3r3k add label: rebuild/windowsRS1
d3r3k add label: rebuild/janky

... and nothing happened, so I figured I should start with a capital D, and edited the comment accordingly:

D3r3k add label: rebuild/windowsRS1
D3r3k add label: rebuild/janky

Expected Behaviour

I expected two labels to be added.

Current Behaviour

It added a single label "rebuild/windowsRS1 D3r3k add label: rebuild/janky" (yes, with a new line (aka carriage return, aka \n, aka %0D) in between).

I tried to remove it by using something like

D3r3k remove label: rebuild/windowsRS1
D3r3k add label: rebuild/janky

but it didn't work. It might be a bug in Github rather than Derek though )

Proposed solution

I see a few possible ways to handle it:

alexellis commented 5 years ago

Hi @kolyshkin

I'm glad that you and the other maintainers are finding Derek useful on the Moby project. I'd like to catch up to hear how you are using it more especially with the command I noticed "(reserved for my Derek commands)" that a few people are using.

As of today Derek commands need to be entered one at a time in separate comments, but if multiple commands make this more usable for you I'd like us to try to accommodate that.

We've approached this ask the past via #44 and #32 but it stalled and stayed in WIP. @rgee0 was leading that effort.

The suggestion you have sounds simpler than how we approached it ("on paper"), but has different tradeoffs because it would involve batching. At the moment each single comment is a single command with a request and response that can be evaluated for pass/fail or success/failure.

Let me ping Richard and see if we can come up with something between us on this issue. Once we get to design/approved, would you be able to work on a PR for this?

Alex

rgee0 commented 5 years ago

Two use cases here:

1) Multiples of the same command 2) Multiple different commands

The two could be solved by a single solution but I think that would still leave room for improvement in the UX in the first use case. e.g. does a user find this productive:

D3r3k add label: rod
D3r3k add label: jane
D3r3k add label: freddie
D3r3k add label: bert
D3r3k add label: ernie

20 words = 5 labels.
I suspect theyd prefer a smaller word to label ratio:

D3r3k add label: rod jane freddie bert ernie

Looking at the commands there is only the labels feature that fits use case 1, hence #44. I think this is still the case 10 months later.

olljanat commented 5 years ago

I'm glad that you and the other maintainers are finding Derek useful on the Moby project.

@alexellis to be accurate I and @kolyshkin have curator role ole Moby and we are able to use Derek to manage issues/PRs.

I'd like to catch up to hear how you are using it more especially with the command I noticed "(reserved for my Derek commands)" that a few people are using.

That is trick which noticed sometime ago. Adding new comment triggers notification for everyone who are following PR but editing existing comments does not do it.

GitHub btw allow you to see old versions of comments if you click "edited" on top of the command.

I suspect theyd prefer a smaller word to label ratio:

D3r3k add label: rod jane freddie bert ernie

Agreed that it would be simpler like that.

@kolyshkin on Moby we actually have also option to use label **rebuild/*** which will trigger all failed builds to rerun but I use it very rarely because then it is very hard to keep track that which CIs have failed most often.

kolyshkin commented 5 years ago

What set me away from the right path is usage examples in README.md (I only found the USER_GUIDE now, and it mentions that multiple commands are not supported!), like this one:

Labels can be used to triage work or help sort it.

D3r3k add label: proposal
D3r3k add label: help wanted
D3r3k remove label: bug

To me, it was very clear from this example that

  1. several commands can be used at once;
  2. in order to add two labels, two commands are needed.

This is why I did what I did and ended up with a weird label that contains a newline and that the bot can't remove.

So (besides the lack of ability to use multiple commands or multiple values) the issues I see are:

README.md is looking like a user guide.

Perhaps it can be shortened and the link to UG be made more visible (or maybe UG can be merged to README)

Examples in README.md are misleading.

I guess they should be either removed, shortened to not have more than a single comment, or rewritten like this:

Labels can be used to triage work or help sort it.

D3r3k add label: proposal
D3r3k add label: help wanted
D3r3k remove label: bug

A newline in input is mishandled

The USER_GUIDE says that "Additional white-space or new-lines is not supported", yet I managed to add a label containing a newline. I think this is a bug, and the obvious solution is to trim the input up to, but not including, the first newline.

I'll work on that once I have time to spare.

alexellis commented 5 years ago

Hi @kolyshkin does #115 satisfy your use-case?

kolyshkin commented 5 years ago

Hi @kolyshkin does #115 satisfy your use-case?

@alexellis I'd say it's a big improvement, although there are still issues remaining:

  1. Mishandling newlines in input (instead of ignoring everything after the first newline, the newline and the text on the second line is added to the label)

  2. Misleading README.md examples (a few commands in a single example gives the idea that this is supported).

rgee0 commented 5 years ago

I've broken the commands up in the README in the latest commit.

Sorting the newlines is in progress.

rgee0 commented 5 years ago

Newlines are now handled - anything after the first line is ignored.