search

mattermost-community / mattermost-plugin-solar-lottery

A tool somewhat similar to pager duty, allows to have rotations with magic "solar lottery" scheduling, or overrides.
Apache License 2.0
4 stars 10 forks source link

Initial implementation #6

Closed levb closed 4 years ago

levb commented 4 years ago

See https://github.com/mattermost/mattermost-plugin-solar-lottery/blob/dev/demo.md

To run the entire demo script on your local instance, type /lotto demo all, requires test-data.

/lotto info for current info.

levb commented 4 years ago

@lindy65 you maybe interested in this as a "beta customer", for the R&D Ice breakers, maybe help test it?

lindy65 commented 4 years ago

Thanks @levb - I'll ask Dylan's help to set up for testing :)

lindy65 commented 4 years ago

Thanks @levb - Dylan helped set up on mysql.test and I tested the plugin. Looks good to me and will be great for ice-breaker! :)

levb commented 4 years ago

Thanks @crspeller for the review, it's a lot of lines, appreciate it!

  • Overall the CLI needs some more work to make it nice to use.

Noted, both structurally (command/flag set) and heuristically, like allowing just a space-separated rotation names and @usernames.

  • A glossary of terms (rotation, shift... etc) would be especially useful.

👍

  • Some work needs to be done around smaller rotations. Got some strange results (like the same user being scheduled repeatedly)

Yes, I saw the same thing just before the demo. Likely de-randomized something in the last refactoring of the scheduler; adding tests should help.

  • It would be nice if the "guess" was stable if no parameters changed.

👍 will add a --randomize or something for the rare other use case.

  • User vs admin commands and help would be nice.

Right. One obvious improvement begging to be made is the concept of rotation admin(s) but sysadmin vs user would be a great step forward, agreed.

Integration with the workflow plugin would be great.

Let's file additional enhancement issues for the necessary APIs?

levb commented 4 years ago

Filed https://github.com/mattermost/mattermost-plugin-solar-lottery/issues/11 and https://github.com/mattermost/mattermost-plugin-solar-lottery/issues/12 filed to address some of the concerns.

I could not reproduce the small rotation weirdness, will try again, will file as separate issues; also need more tests for fill now.

@crspeller @cpoile I'm submitting the feedback as a separate PR, since it's quite massive. It can be re-pointed at master if it makes it easier (lots of refactoring there)

levb commented 4 years ago

This is huge, I'm learning a lot.

Thanks, I did myself in the process. Go is very different from C where it comes to packaging code.

It's interesting -- you're using fields on structs (like in api) to carry parameters into methods, kind of like a context in a request... Yah, that is what you're doing. You're treating slash commands like http requests. Hah, nice. Once I figured that out it became a lot clearer. Maybe we can rename it to make this a bit more clear on first sight? What you're doing feels like it's overloading the meaning of an api struct -- because in other places we've used just to hold api methods or helper methods. This is more like a context.

My thinking - I'd like us to converge at some point in the future where there is a structure like

HTTP and Command interfaces really ought to become one at some point. I'd love for a senior community member/candidate to write an AST-based generator for these, with logging and all. With some creativity.

I am also hesitant to do much there until the HTTP endpoints are actually written and used - we'll see the best packaging then? I don't like the use of gorilla/mux for instance; other things aside it necessitates playing games with Context. One way would be to just rely on Context for everything but my understanding was that it was discouraged when it was designed. Anyway, I feel like it's something to solve later.

I'm using the demo code as examples -- if we're going to push to community we definitely need thorough explanations for the commands, and formatted examples. Like: Here are the steps to set up a team, set up an icebreaker event, set up a SET, give someone time off. The rotation commands are particularly difficult, I think.

I filed https://github.com/mattermost/mattermost-plugin-solar-lottery/issues/11 to create a README. I agree that the documentation and "template use cases" are needed.

As far as improving the overall usability of the CLI, I suggest we wait until we gather experience on community. Having said that, there is one obvious improvement I'd like to make, and that is to rely on parameters (not flags) for rotation, user, and shift references, via a normalized syntax. Anything that is a @.+ is a user, anything that is a \w#[0-9]+ is a shift reference, anything else is a regexp on the rotation name. I'll think about it some more and file a HW ticket.

FWIW, I have mixed feelings at best about pflag, but nothing specific yet.

Is there a way to specify a rotation that has randomness but makes sure that all eligible people are picked i times before someone is picked for their i+1th time? Because that's how I imagine the ice-breaker should work.

No, but there is now separation of concern that should allow for an easy development of a queue Autofiller, which can keep its own persistence, and have its own logic if it wants to be smarter than a simple queue.

Separately, the current userWeight function in solarloterry only takes the LastServed into account. Once user's history is saved (a large-size new ticket?) it should be fully taken into account for scoring. I am still researching and thinking about how to best implement that. It may be the single best remaining improvement to the algorithm itself; its efficiency matters less than fairness IMO.

  • the box profile pic is Jira's

https://github.com/mattermost/mattermost-plugin-solar-lottery/issues/16

levb commented 4 years ago

@cpoile @crspeller I think I addressed both of yours feedback. I took the liberty of resolving the threads that I felt were done, to keep the PR manageable, feel free to reopen as appropriate.

I filed a bunch of follow up tickets, but the biggest obstacles to the community deployment in my mind are:

levb commented 4 years ago

@crspeller, done.

  1. The Autofiller interface has to be defined in the solarlottery package, or it will create a circular dependency - it uses sl entities as parameters, and is needed by sl itself. I did create the tree though, moved Error there, much nicer. Also added a queue placeholder to show the intent. Also, since the top package name is always aliased to keep it short, I don't think duplicate names are an issue, thoughts?
  2. Thanks for suggesting renaming the test files, I ended up restructuring the code instead to match the tests, looks better now.
  3. Period/Duration is not a trivial matter, see my inline comment. Suggest leaving as is.
levb commented 4 years ago

I'm going to merge to master as is, and log a PM review ticket for the community deployment milestone, specifically after there's a README.