Page MenuHomePhabricator

Restructure Drydock so that blueprints are instances in the DB
ClosedPublic

Authored by hach-que on Nov 23 2013, 6:35 AM.
Tags
None
Referenced Files
F13048541: D7638.diff
Thu, Apr 18, 8:52 PM
Unknown Object (File)
Thu, Apr 11, 8:43 AM
Unknown Object (File)
Mon, Apr 8, 1:54 AM
Unknown Object (File)
Fri, Apr 5, 4:54 AM
Unknown Object (File)
Wed, Apr 3, 7:13 PM
Unknown Object (File)
Wed, Apr 3, 7:13 PM
Unknown Object (File)
Wed, Apr 3, 7:13 PM
Unknown Object (File)
Wed, Apr 3, 7:13 PM

Details

Summary

(this diff used to be about applying policies to blueprints)

This restructures Drydock so that blueprints are instances in the DB, with an associated implementation class. Thus resources now have a blueprintPHID instead of blueprintClass and DrydockBlueprint becomes a DAO. The old DrydockBlueprint is renamed to DrydockBlueprintImplementation, and the DrydockBlueprint DAO has a blueprintClass column on it.

This now just implements CAN_VIEW and CAN_EDIT policies for blueprints, although they are probably not enforced in all of the places they could be.

Test Plan

Used the create-resource and lease commands. Closed resources and leases in the UI. Clicked around the new and old lists to make sure everything is still working.

Diff Detail

Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

src/applications/drydock/blueprint/DrydockBlueprint.php
402–420

These all default to POLICY_ADMIN since Drydock is expected to be administrative / internal.

src/view/form/PHUIFormLayoutView.php
17–38

This was copied from AphrontFormView since I needed to append Remarkup to the AJAX form.

@epriestley I'm going to rearchitecture this so that admins can create concrete blueprints in the UI based on a "Blueprint Implementation" (which will be the list of classes actually implemented). This will allow admins to create multiple concrete blueprints from an implementation and thus restrict differents sets of people to doing different things (so I can say that people in the "QA" project have access to one concrete blueprint, it's resources and it's leases, and people in the "Devs" project have access to a different blueprint and it's resources & leases).

I'll unify the policies so that view and edit carry from the blueprint to the resources and leases, imply that CAN_EDIT gives you the ability to lease and resource (and remove the explicit lease and resource policies). If people require the ability to distinguish editing and resourcing/leasing later we can always add the extra policy rules, but we can't do the reverse.

hach-que updated this revision to Unknown Object (????).Nov 30 2013, 5:40 AM

Restructured drydock blueprints to be instance-based.

hach-que updated this revision to Unknown Object (????).Nov 30 2013, 5:42 AM

Remove capabilities.

I've updated the revision title and description to reflect the new purpose of this diff.

I think I've got all correct call sites (by testing the functionality through bin/drydock and the UI), but I can't be sure. In particular, I don't like the fact that half of the code now uses a variable called $blueprint and means "the implementation", and half the code uses $blueprint and means "an instance". The only way to fix this up is to probably manually go through all of the code and change the variable from $blueprint to $implementation where appropriate.

A couple of substantive inlines, but this looks great overall. Thanks!

resources/sql/patches/20131123.drydockblueprintpolicy.sql
10–11

Add:

UNIQUE KEY `key_phid` (phid)

The only other key I can think of which might be useful here is:

KEY `key_class` (className)

Not sure if that's worth adding or not, but I could imagine searching by blueprint type being useful, so maybe we should.

Maybe we should also give these a human-readable name.

src/applications/drydock/blueprint/DrydockBlueprintImplementation.php
43–55

We should maybe invert this eventually (give the Blueprint a BlueprintInstance, not vice-versa) but this is reasonable enough for the moment.

src/applications/drydock/storage/DrydockBlueprint.php
66–71

Either now or in the future, we should get rid of this rule. Generally, I want to make administrators even less powerful than they are now (I plan to eventually strip them of their implicit rights to manage application policies, for example).

src/view/form/PHUIFormLayoutView.php
10–15

AphrontView should already have setUser(). This is a bit inconsistent (it would be nice to realign it to setViewer() for consistency), but we should introduce setViewer() only as a larger effort to do that.

src/applications/drydock/blueprint/DrydockBlueprintImplementation.php
43–55

Generally the process is:

  • Load blueprints from the DB
  • Implicitly create the implementation class against each blueprint
  • Assign the instance to the new implementation class

In this manner, it's already two-way (an implementation has an instance, and an instance has an implementation).