Page MenuHomePhabricator

Feedback form "Audit commit" in Diffusion commits
Closed, DeclinedPublic

Description

Reported upstream: https://secure.phabricator.com/T6630

It looks like Audit was merged with Diffusion upstream, and now we have an "Audit Commit" form at the end of every patch. See for instance

https://phabricator.wikimedia.org/rMW4f8bd08be96b2940c2a1b618a66b679e8c0d9c99

Now we just attached to 1000s of commits a feedback form including novel concepts in our code review process like "Accept commit" and "Raise concern". Since we haven't discussed anything about post-commit review, and developers are not aware of this new channel of communication, ideally we should disabled until the day we agree a plan on it. However, I don't see a way to do so.

Unless I'm missing something... I think we should report this upstream. Having the Audit portion configurable sounds like a reasonable request simple to implement.

Event Timeline

Qgil created this task.Nov 24 2014, 9:16 AM
Qgil raised the priority of this task from to High.
Qgil updated the task description. (Show Details)
Qgil changed Security from none to None.
Qgil added subscribers: Qgil, demon, Jdforrester-WMF and 3 others.
demon added a comment.Nov 24 2014, 3:53 PM

Is it that the applications were merged, or that audit is just better exposed via a textbox on Diffusion now? I still see Audit as an independent application (currently uninstalled).

If it's still its own thing, perhaps this textbox on Diffusion should only show up when Audit's enabled? That would be the most reasonable course of action to me.

Qgil updated the task description. (Show Details)Nov 24 2014, 10:52 PM
Qgil moved this task from Backlog to Wikimedia requests on the Phabricator (Upstream) board.
Qgil added a comment.Dec 9 2014, 2:57 PM

Thanks to T77959 I have learned that trying to comment in a commit results in 404, at least in rEADT7b77eb7970283304554888e2bcf88bcda1f57f0e.

That might be a good interim solution to avoid people commenting in places that might disappear soon.

Qgil lowered the priority of this task from High to Medium.EditedDec 10 2014, 12:34 PM

Strangely enough, the 404 makes this task Normal priority.

Qgil added a comment.Jan 14 2015, 11:26 PM
In T75715#836328, @Qgil wrote:

Strangely enough, the 404 makes this task Normal priority.

The 404 is away, and I was just able to post a comment in rEVEDa2338df67535c7909c045b00fc047021df532fc5

I don't think this is good...

demon added a comment.Jan 15 2015, 2:01 AM

Considering Audit and Diffusion are slowly becoming the same application I'm not too worried. I'm also not sure what the harm is in allowing people to audit commits; it seems like a useful feature to be able to flag a commit for review after it's already been merged.

If we think that opening this new channel causes no harm, then we can resolve this task as Declined, and document the feature so people know what to expect.

The feature in itself is ok, but I agree with Qgil that having yet another place where to comment a commit is disruptive: we don't want to further fragment conversation.

Additionally, if we ever wanted to import CodeReview discussions into diffusion for commits imported from SVN, having recent discussions to merge to the old ones seems likely to complicate things.

Isn't there an option to restrict write permission to admins/nobody?

Nemo_bis renamed this task from Audit feedback form in Diffusion commits to Feedback form "Audit commit" in Diffusion commits.Jan 24 2015, 10:40 AM
demon added a comment.Jan 24 2015, 6:25 PM

Isn't there an option to restrict write permission to admins/nobody?

I think there was a bug here we'd identified. @chasemp?

Without https://secure.phabricator.com/T6630#85907 I am not sure what we can do,. We can tinker with old audit perms to see how they affect.

Personally I find that audit is highly useful for release engineering: When some fatal has slipped through the cracks and I discover an error in the fatalmonitor, I can track it back to the responsible code and raise a concern to get the problem dealt with. See for example: rECOE36abbc628834ae141f9c28f0bc3d2c8d44524754

Personally I find that audit is highly useful for release engineering: When some fatal has slipped through the cracks and I discover an error in the fatalmonitor, I can track it back to the responsible code and raise a concern to get the problem dealt with. See for example: rECOE36abbc628834ae141f9c28f0bc3d2c8d44524754

Except the comments you left there and any events surrounding it were never seen by anyone because I'm not looking there. I didn't see your comment pointing out the problem until two days after I fixed it myself independently.

Personally I find that audit is highly useful for release engineering: When some fatal has slipped through the cracks and I discover an error in the fatalmonitor, I can track it back to the responsible code and raise a concern to get the problem dealt with. See for example: rECOE36abbc628834ae141f9c28f0bc3d2c8d44524754

Except the comments you left there and any events surrounding it were never seen by anyone because I'm not looking there. I didn't see your comment pointing out the problem until two days after I fixed it myself independently.

Are you sure you aren't talking about a different commit? rECOE36abbc was resolved just a bit more than an hour after I raised a concern.

I can't think of any situation where I raised a concern and then it went un-addressed for any significant amount of time. When I'm deploying code and a problem comes up, I don't just raise concern and then wash my hands of it, I investigate the problem, contact the appropriate people on IRC, etc. If something is serious enough that I raise a concern then letting it go for two days would be a fairly serious failure on my part. Is there a different commit that I'm not seeing?

Qgil lowered the priority of this task from Medium to Lowest.Jul 27 2015, 1:10 PM

It's been half year and so far this form seems to be not causing any harm. Even if I was the one reporting this problem (and at the time I thought it would be problematic indeed) now I think that it is fine to keep it. At least until there is a specific concern, abuse, or something along these lines.

Also, we are not going to work on this or change anything unless upstream decides to resolve https://secure.phabricator.com/T6630. Therefore, I'm changing priority accordingly.

mmodell changed the task status from Open to Stalled.Jul 27 2015, 8:04 PM
demon added a comment.Sep 3 2015, 4:13 PM

I'd actually propose declining this outright.

+1 for decline.

Qgil closed this task as Declined.Sep 3 2015, 7:23 PM
Qgil claimed this task.
demon moved this task from To Triage to Done on the Gitblit-Deprecate board.Jan 13 2016, 9:39 PM
Restricted Application added a subscriber: TerraCodes. · View Herald TranscriptMay 23 2016, 6:05 PM