Page MenuHomePhabricator

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

Authored by epriestley on Feb 7 2019, 6:34 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Mar 26, 1:25 PM
Unknown Object (File)
Tue, Mar 26, 1:24 PM
Unknown Object (File)
Tue, Mar 26, 1:24 PM
Unknown Object (File)
Tue, Mar 26, 1:24 PM
Unknown Object (File)
Tue, Mar 26, 1:23 PM
Unknown Object (File)
Tue, Mar 26, 1:23 PM
Unknown Object (File)
Feb 24 2024, 4:58 PM
Unknown Object (File)
Feb 15 2024, 1:05 AM
Subscribers
None

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
Branch
audit3
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 21884
Build 29877: Run Core Tests
Build 29876: arc lint + arc unit

Event Timeline

  • More documentation improvements.
amckinley added inline comments.
src/applications/audit/management/PhabricatorAuditManagementDeleteWorkflow.php
183–185

Why not do this with one big query?

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

"Syncornizing"

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

"if you manually update"

183–184

"may no longer be consistent."?

This revision is now accepted and ready to land.Feb 7 2019, 11:14 PM
src/applications/audit/management/PhabricatorAuditManagementDeleteWorkflow.php
183–185

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.

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

  • 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.Feb 13 2019, 1:45 PM