Page MenuHomePhabricator

Allow repositories to be deleted using `./bin/remove`.
ClosedPublic

Authored by joshuaspence on Jun 2 2014, 11:11 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Dec 19, 7:49 AM
Unknown Object (File)
Fri, Dec 13, 8:52 PM
Unknown Object (File)
Wed, Dec 11, 11:06 AM
Unknown Object (File)
Mon, Dec 9, 5:21 PM
Unknown Object (File)
Mon, Dec 9, 7:42 AM
Unknown Object (File)
Sun, Dec 8, 1:07 PM
Unknown Object (File)
Fri, Dec 6, 11:38 PM
Unknown Object (File)
Tue, Dec 3, 5:12 PM
Subscribers

Details

Summary

Currently, repositories can be deleted using ./bin/repository delete. It makes sense to expose this operate to the ./bin/remove script as well, for consistency.

Test Plan

Deleted a repository with ./bin/remove rTEST.

Diff Detail

Repository
rP Phabricator
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

joshuaspence retitled this revision from to Allow repositories to be deleted using `./bin/remove`..
joshuaspence updated this object.
joshuaspence edited the test plan for this revision. (Show Details)
joshuaspence added a reviewer: epriestley.
epriestley edited edge metadata.

This destruction is strictly better than bin/repository delete since it automatically cleans up common linked infrastructure, so let's get rid of the old thing:

  • Remove bin/repository delete (or leave it alive with a deprecation message).
  • Change the UI hint about bin/repository delete to point here instead (there's a dialog that pops when you try to delete from the web UI).
This revision now requires changes to proceed.Jun 2 2014, 11:17 PM
In D9350#6, @epriestley wrote:

This destruction is strictly better than bin/repository delete since it automatically cleans up common linked infrastructure, so let's get rid of the old thing:

  • Remove bin/repository delete (or leave it alive with a deprecation message).
  • Change the UI hint about bin/repository delete to point here instead (there's a dialog that pops when you try to delete from the web UI).

I think I'm missing something here... how is this method better? It is basically just calling the same $this->delete() function? Does PhabricatorDestructionEngine handle delete transaction and edges related to the deleted object?

I did notice this TODO comment in the PhabricatorLiskDAO class:

public function delete() {

  // TODO: We should make some reasonable effort to destroy related
  // infrastructure objects here, like edges, transactions, custom field
  // storage, flags, Phrequent tracking, tokens, etc. This doesn't need to
  // be exhaustive, but we can get a lot of it pretty easily.

  return parent::delete();
}

Is this TODO still relevant if the PhabricatorDestructionEngine does this stuff? (I had started to work on implementing this TODO, but can abandon that if it is no longer required)

PhabricatorDestructionEngine handles that stuff, yes (and some level of logging, too). It doesn't handle everything yet, but does do Transactions and Edges. There are a couple of TODOs about other infrastructure systems.

The LiskDAO comment is obsolete with the advent of PhabricatorDestructionEngine. If you have work on subsystems not covered by the existing DestructionEngine code (for instance, it doesn't do tokens), that stuff can move there.

joshuaspence edited edge metadata.
  • Recommend ./bin/remove destroy instead of ./bin/repository delete.
  • Remove the ./bin/repository delete workflow.
epriestley edited edge metadata.
This revision is now accepted and ready to land.Jun 3 2014, 12:11 AM
epriestley updated this revision to Diff 22271.

Closed by commit rP0d03bbe43cf7 (authored by @joshuaspence, committed by @epriestley).