Page MenuHomePhabricator

Make "bin/audit delete" synchronize commit audit status, and improve "bin/audit synchronize" documentation
ClosedPublic

Authored by epriestley on Thu, Feb 7, 6:34 PM.

Details

Summary

Depends on D20126. See PHI1056. Ref T13244.

  • bin/audit delete destroys audit requests, but does not update the overall audit state for associated commits. For example, if you destroy all audit requests for a commit, it does not move to "No Audit Required".
  • bin/audit synchronize does this synchronize step, but is poorly documented.

Make bin/audit delete synchronize affected commits.

Document bin/audit synchronize better.

There's some reasonable argument that bin/audit synchronize perhaps shouldn't exist, but it does let you recover from an accidentally (or intentionally) mangled database state. For now, let it live.

Test Plan
  • Ran bin/audit delete, saw audits destroyed and affected commits synchornized.
  • Ran bin/audit synchronize, saw behavior unchanged.
  • Ran bin/audit help, got better help.

Diff Detail

Repository
rP Phabricator
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

epriestley created this revision.Thu, Feb 7, 6:34 PM
epriestley requested review of this revision.Thu, Feb 7, 6:36 PM
epriestley updated this revision to Diff 48047.Thu, Feb 7, 6:39 PM
  • More documentation improvements.
amckinley accepted this revision.Thu, Feb 7, 11:14 PM
amckinley added inline comments.
src/applications/audit/management/PhabricatorAuditManagementDeleteWorkflow.php
170–172

Why not do this with one big query?

src/applications/audit/management/PhabricatorAuditManagementWorkflow.php
116

"Syncornizing"

src/docs/user/userguide/audit.diviner
182–184

"if you manually update"

183–184

"may no longer be consistent."?

This revision is now accepted and ready to land.Thu, Feb 7, 11:14 PM
epriestley added inline comments.Thu, Feb 7, 11:23 PM
src/applications/audit/management/PhabricatorAuditManagementDeleteWorkflow.php
170–172

I'm hoping to cheat and sneak under conditions where we explode by using too much memory, if you delete 1M audits with this script or something.

At some point, both this and synchronize probably need to be rewritten to use Iterators instead. Maybe I'll just do that now since it sounds like the immediate problems found workarounds.

epriestley planned changes to this revision.Fri, Feb 8, 12:50 AM

Let me try to make a somewhat more convincing effort here.

epriestley updated this revision to Diff 48136.Wed, Feb 13, 1:45 PM
  • Fix typos.
  • Use a QueryIterator.

We're still holding a small record in memory for each affected audit, so we can show the user a list of all affected audits and then prompt them to continue, but our peak memory usage is approximately as small as possible without making workflow changes.

This revision is now accepted and ready to land.Wed, Feb 13, 1:45 PM
This revision was automatically updated to reflect the committed changes.