Decision Process Implementation #1680
Conversation
* add comment builder for the decision state * improve error handling * add factory for user status and add tests * cleanup unnecessary code
* Parse team in command * Use parsed team in handler
* Move dependencies only to dev * Limit to team member when initializing the process * Display @ on user names in table
| #[postgres(name = "merge")] | ||
| Merge, | ||
| #[postgres(name = "hold")] | ||
| Hold, |
There was a problem hiding this comment.
I think to start we should try to mostly match rfcbot, which largely supports two resolutions: merge and close. We can extend further in the future though.
There was a problem hiding this comment.
Agree on adding close command but we should keep hold as specified in the doc we based our impl: https://lang-team.rust-lang.org/decision_process/examples.html, because it's the way to "pause" the process, wdyt?
In the current code, hold is not doing that either so will need to fix it as well
There was a problem hiding this comment.
I'm fine with extending the process a little per the proposal, though let's avoid delaying too much as a result.
| #[derive(Debug, Eq, PartialEq)] | ||
| pub struct DecisionCommand { | ||
| pub resolution: Resolution, | ||
| pub reversibility: Reversibility, |
There was a problem hiding this comment.
I would probably drop reversibility from this first iteration -- we'll want it eventually, but we don't need it for the core logic and if we do have it we'll want a way to set it I think...
| @@ -273,4 +274,20 @@ CREATE UNIQUE INDEX jobs_name_scheduled_at_unique_index | |||
| name, scheduled_at | |||
| ); | |||
| ", | |||
| " | |||
| CREATE TYPE reversibility AS ENUM ('reversible', 'irreversible'); | |||
There was a problem hiding this comment.
This feels like it'll be needlessly restrictive going forward, and I at least am not familiar enough with postgres types to be confident that we're not adding some mostly one-way door decision here. Can we stick with storing this in just a text field?
It's not clear to me whether we ever actually need to query by this field, in which case just adding it to a single JSON blob we store for these may be better -- then we can even store all of this into issue data, rather than a new table. We should never have so many FCPs they don't fit in memory etc.
There was a problem hiding this comment.
Yeah agree, we can add a jsonb data field and store just resolution there for now, and in a future iteration add reversibility and more. Wdyt?
* Fix top comment in decision parser * Display vote with link to the event that caused it
joulei
force-pushed
the
decision-process-impl
branch
2 times, most recently
from
9c72e82
to
555cb07
Compare
Co-authored by @mcass19
This a working draft for the decision process implementation.
Our intent is to share this POC with merge and hold to get a first round of feedback.
As of now, it only processes the first voting command.
We're leaving out commented the scheduled job insertions and such.
Next steps are: