Page MenuHomePhabricator

Uninstalling Audit should either be impossible or interact more cleanly with Diffusion
Closed, ResolvedPublic

Description

We enabled Diffusion in https://phabricator.wikimedia.org/diffusion/ with the intention of getting a code repository browser. And we got that, but we also got a full fledged "Audit Commit" form at the end of every commit page, including concepts like "Accept commit" and "Raise concern". We have never used Audit, it is not even installed in our instance.

As a community we haven't discussed the possibility to open a new channel of communication for post-commit code review. We love Diffusion, we want to get rid of GitBlit thanks to it, we are importing new repositories every week... but we want to get rid of those forms as soon as possible to avoid confusion with our actual code review process (pre-commit, via Gerrit until the day we figure out our migration to Differential).

Original report: https://phabricator.wikimedia.org/T75715

Event Timeline

qgil created this task.Nov 24 2014, 10:50 PM
qgil raised the priority of this task from to Needs Triage.
qgil updated the task description. (Show Details)
qgil added projects: Wikimedia, Diffusion, Audit.
qgil updated the task description. (Show Details)
qgil moved this task from Backlog to Important on the Wikimedia board.
qgil added a subscriber: qgil.

@chad / @btrahan, do you think this is worth separating? Offhand, the integration points I can think of are:

  • Ability to comment and add inlines;
  • actual Audit UI (probably already covered by uninstalling Audit);
  • audit-triggering Herald rules;
  • "Auditors:" text rule in commit messages;
  • I think there's an "automatic audit" trigger in Owners.

It seems fairly reasonable to me to gate all of these on "Audit" actually being installed.

The counterargument is that we've generally been merging Audit into Diffusion and I'm not sure they're really two separate applications anymore at this point, or if they'll remain separate in the future (I'd like to collapse the ancient Repository UI completely into Diffusion, and possibly collapse all of the Audit UI into it, too).

Another thrust would be separating permissions ("Can Comment" vs "Can View") but I'd like stronger use cases for this.

On the WMF side, if we fail to reach and enact a favorable consensus here quickly, if you just commented out this form locally that would probably get you most of the way there. The other integrations are difficult to stumble across or irrelevant because of other configuration.

chad added a comment.Nov 24 2014, 11:02 PM

It seems reasonable for them to be separate.

I don't really care whether the application is separate or inside some super-Diffusion; I guess I have a modest preference for super diffusion because it feels like organic audits happen while browsing the code and otherwise you'd just go to audit when you knew there was a problem or something?

I do think you should be able to easily turn off audit review flows. I think we should actually consider shipping with these off by default to get a bit more "paternal" on how we want code review to work.

qgil added a comment.Dec 2 2014, 8:13 AM

Alright, we seem to agree on the idea of merging Audit under Diffusion, yet leaving a configuration option to enable/disable the post-commit review workflow.

eadler added a subscriber: eadler.Apr 27 2015, 6:18 PM
qgil added a comment.Sep 3 2015, 7:24 PM

For your information, we have closed the original report of this task at https://phabricator.wikimedia.org/T75715 as Declined.

epriestley renamed this task from Audit form in Diffusion should be optional to Uninstalling Audit should either be impossible or interact more cleanly with Diffusion.Mar 27 2016, 5:01 PM
epriestley added a subscriber: swisspol.
epriestley moved this task from Backlog to EditEngine on the Audit board.Jan 10 2017, 4:44 PM

Although I earlier (in T6630#85282) felt like it was reasonable to consider separating Audit and Diffusion, there are now more existing or planned integration points and I think this approach is significantly less reasonable than the "merge them" approach. It also doesn't seem to be causing any problems on real installs.

Thus, I'm planning to resolve this by removing the ability to uninstall Audit (since the existence of that capability is misleading) and generally continuing to merge Audit into Diffusion.