From e1fc2abaa3ba66241b049915aab9c385565b71a4 Mon Sep 17 00:00:00 2001
From: Mike Kenyon <mkenyon@pivotal.io>
Date: Tue, 26 Mar 2019 15:01:54 -0700
Subject: [PATCH] Add ADR preferring machinist over factory bot

[Finishes #164834657]

Co-authored-by: Eli Wrenn <ewrenn@pivotal.io>
Co-authored-by: Mike Kenyon <mkenyon@pivotal.io>
---
 .adr-dir                                      |  1 +
 .../0001-record-architecture-decisions.md     | 19 ++++
 .../0002-using-machinist-for-factories.md     | 97 +++++++++++++++++++
 3 files changed, 117 insertions(+)
 create mode 100644 .adr-dir
 create mode 100644 decisions/0001-record-architecture-decisions.md
 create mode 100644 decisions/0002-using-machinist-for-factories.md

diff --git a/.adr-dir b/.adr-dir
new file mode 100644
index 00000000000..d9ac8907a81
--- /dev/null
+++ b/.adr-dir
@@ -0,0 +1 @@
+decisions
diff --git a/decisions/0001-record-architecture-decisions.md b/decisions/0001-record-architecture-decisions.md
new file mode 100644
index 00000000000..411357617d1
--- /dev/null
+++ b/decisions/0001-record-architecture-decisions.md
@@ -0,0 +1,19 @@
+# 1. Record architecture decisions
+
+Date: 2019-03-26
+
+## Status
+
+Accepted
+
+## Context
+
+We need to record the architectural decisions made on this project.
+
+## Decision
+
+We will use Architecture Decision Records, as [described by Michael Nygard](http://thinkrelevance.com/blog/2011/11/15/documenting-architecture-decisions).
+
+## Consequences
+
+See Michael Nygard's article, linked above. For a lightweight ADR toolset, see Nat Pryce's [adr-tools](https://github.com/npryce/adr-tools).
diff --git a/decisions/0002-using-machinist-for-factories.md b/decisions/0002-using-machinist-for-factories.md
new file mode 100644
index 00000000000..41937169036
--- /dev/null
+++ b/decisions/0002-using-machinist-for-factories.md
@@ -0,0 +1,97 @@
+2: Using Machinist for Factories
+================================
+
+Date: 2019-03-22
+
+Status
+------
+
+Accepted
+
+
+Context
+-------
+
+[machinist][] is a ruby library that makes it easy to create objects for use in test.
+[Factories tend to be preferable to fixtures][factories-not-fixtures].
+
+We decided to try switching over from [machinist][] to [factory_bot][].
+Our primary decision behind switching was because machinist is unmaintained.
+The last commit to _master_ is from 2013. factory_bot is well maintained
+and is very popular.
+
+We had converted roughly a dozen factories over from machinist to factory_bot,
+starting from the leaf nodes of our object graph.
+As we moved to objects with associations,
+we encountered increasing degrees of friction.
+
+The differences in how [sequel][] and [active_record][] handle associations
+mean that factory_bot was not well suited.
+In active_record,
+associations are created by placing a foreign key on one of the two records.
+Specifically,
+the record with the `belongs_to` contains the foreign key for the record with the `has_one`.
+This means that any factory library can create the `has_one` record,
+generate its primary key 'A',
+then create the `belongs_to` record with a foreign key of 'A'.
+In our usage of sequel,
+some associations have mutual foreign keys on both of the two records.
+So, a factory library has to create the first record,
+generate its primary key 'A' with no association,
+then create the second record with primary key 'B' and foreign key 'A',
+then update the first record so that its foreign key is 'B'.
+This "create-create-update" workflow is tricky (but possible) in factory_bot
+and is clean in machinist.
+
+```ruby
+# machinist
+AppModel.blueprint do
+  name       { Sham.name }
+  space      { Space.make }
+  buildpack_lifecycle_data { BuildpackLifecycleDataModel.make(app: object.save) }
+end
+
+# factory_bot
+FactoryBot.define do
+  factory :app, aliases: [:app_model], class: VCAP::CloudController::AppModel do
+    name
+
+    transient do
+      space
+    end
+
+    trait :buildpack do
+      after(:create) do |app, evaluator|
+        app.buildpack_lifecycle_data = create(:buildpack_lifecycle_data)
+      end
+    end
+
+    trait :docker do
+    end
+
+    after(:create) do |app, evaluator|
+      app.space = evaluator.space if evaluator.space
+    end
+  end
+end
+```
+
+Decision
+--------
+
+We have reverted the conversion commits, remaining with machinist.
+We will switch to a maintained fork of machinist.
+(It turns out that other people use machinist too.)
+
+
+Consequences
+------------
+
+New team members will have to learn machinist,
+whereas rubyists would probably be familiar with factory_bot.
+
+There will no longer be a state where both machinist and factory_bot are used while converting.
+
+[machinist]: https://github.com/notahat/machinist
+[factories-not-fixtures]: http://www.betterspecs.org/#factories
+[factory_bot]: https://github.com/thoughtbot/factory_bot