From 98a2f5adae77f796f07359a83701db990054bf1f Mon Sep 17 00:00:00 2001
From: Christopher Maier <chris@operable.io>
Date: Thu, 25 Aug 2016 11:43:08 -0400
Subject: [PATCH 1/2] Define new template specification

Previously, bundles could include both Slack and HipChat templates. With
the new templating engine, we now only need one template (the specific
chat provider implementations will be responsible for rendering the
template in a manner appropriate to the chat platform).

Now, templates look like this:

```
...
"templates": {
  "template1": {"body": "template1 body"},
  "template2": {"body": "template2 body"},
  ...
}
...

```

Note now that template names are also constrained to the regex
`[A-Za-z0-9_]+`.

This sets the current bundle definition version to 4, and officially
removes support for version 2 bundles. Version 3 is now officially
deprecated.
---
 lib/spanner/config.ex                         |  24 +--
 lib/spanner/config/syntax_validator.ex        |   7 +-
 lib/spanner/config/upgrader.ex                | 119 --------------
 ...a_v2.yaml => bundle_config_schema_v4.yaml} |  38 +++--
 ...lidator_test.exs => v3_validator_test.exs} |  35 +----
 test/spanner/config/v4_validator_test.exs     | 145 ++++++++++++++++++
 6 files changed, 177 insertions(+), 191 deletions(-)
 delete mode 100644 lib/spanner/config/upgrader.ex
 rename priv/schemas/{bundle_config_schema_v2.yaml => bundle_config_schema_v4.yaml} (81%)
 rename test/spanner/config/{validator_test.exs => v3_validator_test.exs} (80%)
 create mode 100644 test/spanner/config/v4_validator_test.exs

diff --git a/lib/spanner/config.ex b/lib/spanner/config.ex
index 93d6461..884738a 100644
--- a/lib/spanner/config.ex
+++ b/lib/spanner/config.ex
@@ -1,14 +1,13 @@
 defmodule Spanner.Config do
   alias Spanner.Config.SyntaxValidator
   alias Spanner.Config.SemanticValidator
-  alias Spanner.Config.Upgrader
 
   # Currently we support version n and n-1. Adding new optional fields
   # does not require a version bump, but adding new mandatory fields,
   # or changing the overall structure of the configuration file does
   # require a bump
 
-  @current_config_version 3
+  @current_config_version 4
   @old_config_version @current_config_version - 1
   @config_extensions [".yaml", ".yml", ".json"]
   @config_file "config"
@@ -61,7 +60,8 @@ defmodule Spanner.Config do
   """
   @spec validate(Map.t) ::
     {:ok, Map.t} | {:error, List.t, List.t} | {:warning, Map.t, List.t}
-  def validate(%{"cog_bundle_version" => @current_config_version}=config) do
+  def validate(%{"cog_bundle_version" => version}=config)
+    when version >= @old_config_version do
     case SyntaxValidator.validate(config) do
       :ok ->
         config = fixup_rules(config)
@@ -75,24 +75,6 @@ defmodule Spanner.Config do
         {:error, errors, []}
     end
   end
-  def validate(%{"cog_bundle_version" => @old_config_version}=config) do
-    # Upgrader will return an upgraded config and a list of warnings
-    # or an error
-    case Upgrader.upgrade(config) do
-      {:ok, upgraded_config, warnings} ->
-        # We still need to validate the upgraded config
-        case validate(upgraded_config) do
-          {:ok, validated_config} ->
-            # If everything goes well, we return the validated config
-            # and a list of warnings.
-            {:warning, validated_config, warnings}
-          {:error, errors, _} ->
-            {:error, errors, warnings}
-        end
-      {:error, errors, warnings} ->
-        {:error, errors, warnings}
-    end
-  end
   def validate(%{"cog_bundle_version" => version}) do
     {:error,
      [{"""
diff --git a/lib/spanner/config/syntax_validator.ex b/lib/spanner/config/syntax_validator.ex
index 431019a..5366ac3 100644
--- a/lib/spanner/config/syntax_validator.ex
+++ b/lib/spanner/config/syntax_validator.ex
@@ -19,9 +19,10 @@ defmodule Spanner.Config.SyntaxValidator do
   @doc """
   Accepts a config map and validates syntax.
   """
-  @spec validate(Map.t, integer()) :: :ok | {:error, [{String.t, String.t}]}
-  def validate(config, version \\ @current_config_version) do
-    with {:ok, schema} <- load_schema(version),
+  @spec validate(Map.t) :: :ok | {:error, [{String.t, String.t}]}
+  def validate(config) do
+    with version <- Map.fetch!(config, "cog_bundle_version"),
+         {:ok, schema} <- load_schema(version),
          {:ok, resolved_schema} <- resolve_schema(schema),
          :ok <- ExJsonSchema.Validator.validate(resolved_schema, config),
        do: :ok
diff --git a/lib/spanner/config/upgrader.ex b/lib/spanner/config/upgrader.ex
deleted file mode 100644
index 2572920..0000000
--- a/lib/spanner/config/upgrader.ex
+++ /dev/null
@@ -1,119 +0,0 @@
-defmodule Spanner.Config.Upgrader do
-  alias Spanner.Config.SyntaxValidator
-
-  @moduledoc """
-  Attempts to upgrade bundle config to current version.
-  """
-
-  @current_version Spanner.Config.current_config_version
-  @upgradable_version @current_version - 1
-
-  @doc """
-  When possible upgrades config to the current version. Returns the upgraded
-  config and a list of warnings for deprecated config options or an error if
-  the upgrade fails.
-  """
-  @spec upgrade(Map.t) :: {:ok, Map.t, List.t} | {:error, List.t}
-  def upgrade(%{"cog_bundle_version" => @upgradable_version}=config) do
-    deprecation_msg =
-      {"""
-       Bundle config version #{@upgradable_version} has been deprecated. \
-       Please update to version #{@current_version}.\
-       """,
-       "#/cog_bundle_version"}
-    # We run the validator for the old version here. So if the user passes an
-    # old version that is also invalid, we don't crash trying to access fields
-    # that don't exist.
-    case SyntaxValidator.validate(config, @upgradable_version) do
-      :ok ->
-        do_upgrade(config)
-        |> insert_deprecation_msg(deprecation_msg)
-      {:error, errors} ->
-        {:error, errors, [deprecation_msg]}
-    end
-  end
-
-  defp do_upgrade(config) do
-    case execution_once?(config["commands"]) do
-      {true, {warnings, errors}} ->
-        {:error, errors, warnings}
-      {false, warnings} ->
-        {enforce_warnings, updated_commands} = update_enforcing(config["commands"])
-        updated_config = %{config | "commands" => updated_commands}
-        |> Map.put("cog_bundle_version", @current_version)
-        {:ok, updated_config, warnings ++ enforce_warnings}
-    end
-  end
-
-  # Checks to see if the execution field exists in the bundle config. If it
-  # does but contains "multiple" we just return a warning. If it contains
-  # "once" we return an error.
-  defp execution_once?(commands) do
-    {warnings, errors} = Enum.reduce(commands, {[],[]},
-      fn
-        ({cmd_name, cmd}, {warnings, errors}) ->
-          case Map.get(cmd, "execution", nil) do
-            "once" ->
-              msg = "Execution 'once' commands are no longer supported"
-              location = "#/commands/#{cmd_name}/execution"
-              updated_errors = [{msg, location} | errors]
-              {warnings, updated_errors}
-            "multiple" ->
-              msg = """
-              Execution type has been deprecated. \
-              Please update your bundle config to version #{@current_version}.\
-              """
-              location = "#/commands/#{cmd_name}/execution"
-              updated_warnings = [{msg, location} | warnings]
-              {updated_warnings, errors}
-            nil ->
-              {warnings, errors}
-          end
-      end)
-
-    if length(errors) > 0 do
-      {true, {warnings, errors}}
-    else
-      {false, warnings}
-    end
-
-  end
-
-  # Updates for non enforcing commands. If 'enforcing: false' is specified we
-  # add the "allow" rule, delete the enforcing field and return a warning. If
-  # 'enforcing: true' is specified we return a warning and delete the field.
-  defp update_enforcing(commands) do
-    Enum.reduce(commands, {[], commands},
-      fn
-        ({cmd_name, %{"enforcing" => enforcing}=cmd}, {warnings, commands}) when not(enforcing) ->
-          msg = """
-          Non-enforcing commands have been deprecated. \
-          Please update your bundle config to version #{@current_version}.\
-          """
-          updated_warnings = [{msg, "#/commands/#{cmd_name}/enforcing"} | warnings]
-
-          updated_cmd = Map.put(cmd, "rules", ["allow"])
-          |> Map.delete("enforcing")
-          updated_commands = Map.put(commands, cmd_name, updated_cmd)
-
-          {updated_warnings, updated_commands}
-        ({cmd_name, %{"enforcing" => _}=cmd}, {warnings, commands}) ->
-          msg = """
-          The 'enforcing' field has been deprecated. \
-          Please update your bundle config to version #{@current_version}.\
-          """
-          updated_warnings = [{msg, "#/commands/#{cmd_name}/enforcing"} | warnings]
-
-          updated_cmd = Map.delete(cmd, "enforcing")
-          updated_commands = Map.put(commands, cmd_name, updated_cmd)
-
-          {updated_warnings, updated_commands}
-        (_, acc) ->
-          acc
-      end)
-  end
-
-  defp insert_deprecation_msg({status, errors, warnings}, msg) do
-    {status, errors, [msg | warnings]}
-  end
-end
diff --git a/priv/schemas/bundle_config_schema_v2.yaml b/priv/schemas/bundle_config_schema_v4.yaml
similarity index 81%
rename from priv/schemas/bundle_config_schema_v2.yaml
rename to priv/schemas/bundle_config_schema_v4.yaml
index 04fda7b..414eb0f 100644
--- a/priv/schemas/bundle_config_schema_v2.yaml
+++ b/priv/schemas/bundle_config_schema_v4.yaml
@@ -1,6 +1,6 @@
 ---
 "$schema": http://json-schema.org/draft-04/schema#
-title: Bundle Config v2
+title: Bundle Config v3
 description: A config schema for bundles
 type: object
 required:
@@ -8,14 +8,15 @@ required:
   - name
   - version
   - commands
-additionalProperties: false
 properties:
   cog_bundle_version:
     type: number
     enum:
-      - 2
+      - 4
   name:
     type: string
+  description:
+    type: string
   version:
     type: string
     pattern: ^\d+\.\d+($|\.\d+$)
@@ -28,15 +29,23 @@ properties:
     required:
       - image
       - tag
+    optional:
+      - binds
     properties:
       image:
         type: string
       tag:
         type: string
+      binds:
+        type: array
+        items:
+          type: string
   templates:
     type: object
-    additionalProperties:
-      "$ref": "#/definitions/template"
+    patternProperties:
+      "^[A-Za-z0-9_]+$":
+        "$ref": "#/definitions/template"
+    additionalProperties: false
   commands:
     type: object
     additionalProperties:
@@ -46,27 +55,24 @@ properties:
 definitions:
   template:
     type: object
-    additionalProperties: false
     properties:
-      slack:
-        type: string
-      hipchat:
+      body:
         type: string
+    required:
+      - body
+    additionalProperties: false
   command:
     type: object
     required:
       - executable
+      - rules
     properties:
       executable:
         type: string
+      description:
+        type: string
       documentation:
         type: string
-      execution:
-        enum:
-          - once
-          - multiple
-      enforcing:
-        type: boolean
       rules:
         type: array
         items:
@@ -86,6 +92,7 @@ definitions:
     type: object
     required:
       - type
+    additionalProperties: false
     properties:
       type:
         type: string
@@ -102,4 +109,3 @@ definitions:
         type: boolean
       short_flag:
         type: string
-
diff --git a/test/spanner/config/validator_test.exs b/test/spanner/config/v3_validator_test.exs
similarity index 80%
rename from test/spanner/config/validator_test.exs
rename to test/spanner/config/v3_validator_test.exs
index 507e637..fb50e32 100644
--- a/test/spanner/config/validator_test.exs
+++ b/test/spanner/config/v3_validator_test.exs
@@ -1,4 +1,4 @@
-defmodule Spanner.Config.Validator.Test do
+defmodule Spanner.Config.V3Validator do
   use ExUnit.Case, async: true
   alias Spanner.Config
 
@@ -26,19 +26,6 @@ defmodule Spanner.Config.Validator.Test do
     }
   end
 
-  defp old_config do
-    %{"cog_bundle_version" => 2,
-      "name" => "foo",
-      "version" => "0.0.1",
-      "commands" => %{
-        "date" => %{
-          "executable" => "/bin/date",
-          "enforcing" => false
-        }
-      }
-    }
-  end
-
   defp incomplete_rules_config do
     %{"cog_bundle_version" => 3,
       "name" => "foo",
@@ -67,12 +54,12 @@ defmodule Spanner.Config.Validator.Test do
   test "wrong cog_bundle_version" do
     result = update_in(minimal_config, ["cog_bundle_version"], fn(_) -> 1 end)
     |> validate
-    assert result == {:error, [{"cog_bundle_version 1 is not supported. Please update your bundle config to version 3.", "#/cog_bundle_version"}], []}
+    assert result == {:error, [{"cog_bundle_version 1 is not supported. Please update your bundle config to version 4.", "#/cog_bundle_version"}], []}
   end
 
   test "missing cog_bundle_version" do
     result = Map.delete(minimal_config, "cog_bundle_version") |> validate
-    assert result == {:error, [{"cog_bundle_version not specified. You must specify a valid bundle version. The current version is 3.", "#/cog_bundle_version"}], []}
+    assert result == {:error, [{"cog_bundle_version not specified. You must specify a valid bundle version. The current version is 4.", "#/cog_bundle_version"}], []}
   end
 
   test "incomplete rules" do
@@ -132,22 +119,6 @@ defmodule Spanner.Config.Validator.Test do
     assert response == {:error, [{"Schema does not allow additional properties.", "#/templates/foo/meh"}], []}
   end
 
-  test "upgrading bundle versions" do
-    response = validate(old_config)
-    {:warning, config, warnings} = response
-
-    assert %{"cog_bundle_version" => 3,
-             "name" => "foo",
-             "version" => "0.0.1",
-             "commands" => %{
-               "date" => %{
-                 "executable" => "/bin/date",
-                 "rules" => ["when command is foo:date allow"]}},
-             } = config
-    assert [{"Bundle config version 2 has been deprecated. Please update to version 3.", "#/cog_bundle_version"},
-            {"Non-enforcing commands have been deprecated. Please update your bundle config to version 3.", "#/commands/date/enforcing"}] = warnings
-  end
-
   # env_vars can be strings, booleans and numbers
   Enum.each(["string", true, 4], fn(type) ->
     test "env_vars can be a #{type}" do
diff --git a/test/spanner/config/v4_validator_test.exs b/test/spanner/config/v4_validator_test.exs
new file mode 100644
index 0000000..7295624
--- /dev/null
+++ b/test/spanner/config/v4_validator_test.exs
@@ -0,0 +1,145 @@
+defmodule Spanner.Config.V4ValidatorTest do
+  # This is effectively the same as the v3 validator test, but with
+  # altered template tests, since that's the only difference between
+  # the two versions.
+
+  use ExUnit.Case, async: true
+  alias Spanner.Config
+
+  defp validate(config) do
+    case Config.validate(config) do
+      {:ok, _} ->
+        :ok
+      error ->
+        error
+    end
+  end
+
+  defp minimal_config do
+    %{"cog_bundle_version" => 4,
+      "name" => "foo",
+      "version" => "0.0.1",
+      "commands" => %{
+        "date" => %{
+          "executable" => "/bin/date",
+          "rules" => [
+            "allow"
+          ]
+        }
+      }
+    }
+  end
+
+  defp incomplete_rules_config do
+    %{"cog_bundle_version" => 4,
+      "name" => "foo",
+      "version" => "0.1",
+      "permissions" => ["foo:view"],
+      "commands" => %{
+        "bar" => %{"executable" => "/bin/bar",
+                   "rules" => ["must have foo:view"]},
+        "baz" => %{"executable" => "/bin/baz",
+                   "rules" => ["allow"]}}}
+  end
+
+  test "minimal config" do
+    assert validate(minimal_config) == :ok
+  end
+
+  test "bad bundle versions" do
+    updated = put_in(minimal_config, ["version"], "1")
+    assert validate(updated) == {:error, [{"String \"1\" does not match pattern \"^\\\\d+\\\\.\\\\d+($|\\\\.\\\\d+$)\".", "#/version"}], []}
+    updated = put_in(minimal_config, ["version"], "0.0.1-beta")
+    assert validate(updated) == {:error, [{"String \"0.0.1-beta\" does not match pattern \"^\\\\d+\\\\.\\\\d+($|\\\\.\\\\d+$)\".", "#/version"}], []}
+  end
+
+  test "wrong cog_bundle_version" do
+    result = update_in(minimal_config, ["cog_bundle_version"], fn(_) -> 1 end)
+    |> validate
+    assert result == {:error, [{"cog_bundle_version 1 is not supported. Please update your bundle config to version 4.", "#/cog_bundle_version"}], []}
+  end
+
+  test "missing cog_bundle_version" do
+    result = Map.delete(minimal_config, "cog_bundle_version") |> validate
+    assert result == {:error, [{"cog_bundle_version not specified. You must specify a valid bundle version. The current version is 4.", "#/cog_bundle_version"}], []}
+  end
+
+  test "incomplete rules" do
+    assert validate(incomplete_rules_config) == :ok
+  end
+
+  test "rules are fixed up" do
+    {:ok, config} = Spanner.Config.validate(incomplete_rules_config)
+    [rule] = get_in(config, ["commands", "bar", "rules"])
+    assert String.starts_with?(rule, "when command is foo:bar")
+  end
+
+  test "errors when permissions don't match rules" do
+    rules = ["when command is foo:date must have foo:view"]
+    config = put_in(minimal_config, ["commands", "date", "rules"], rules)
+    response = validate(config)
+
+    assert response == {:error, [{"The permission 'foo:view' is not in the list of permissions.", "#/commands/date/rules/0"}], []}
+  end
+
+  test "errors on bad rule" do
+    rules = ["when command is foo:bar must have permission == foo:baz"]
+    config = put_in(minimal_config, ["commands", "date", "rules"], rules)
+
+    response = validate(config)
+
+    assert response == {:error, [{"(Line: 1, Col: 34) References to permissions must be the literal \"allow\" or start with a command bundle name or \"site\".", "#/commands/date/rules/0"}], []}
+  end
+
+  test "rules are required" do
+    config = update_in(minimal_config, ["commands", "date"], &Map.delete(&1, "rules"))
+    |> validate
+    assert config == {:error, [{"Required property rules was not present.", "#/commands/date"}], []}
+  end
+
+  test "errors on bad command option type" do
+    options = %{"option_1" => %{"type" => "integer", "required" => false}}
+    config = put_in(minimal_config, ["commands", "date", "options"], options)
+    response = validate(config)
+
+    assert response == {:error, [{"Value \"integer\" is not allowed in enum.", "#/commands/date/options/option_1/type"}], []}
+  end
+
+  # env_vars can be strings, booleans and numbers
+  Enum.each(["string", true, 4], fn(type) ->
+    test "env_vars can be a #{type}" do
+      env_var = %{"env1" => unquote(type)}
+      config = put_in(minimal_config, ["commands", "date", "env_vars"], env_var)
+
+      response = validate(config)
+      assert response == :ok
+    end
+  end)
+
+  ########################################################################
+  # Templates
+
+  test "templates are validated" do
+    templates = %{"blah" => %{"body" => "blahblah"},
+                  "foo" => %{"body" => "foofoo"}}
+    config = put_in(minimal_config, ["templates"], templates)
+    assert :ok == validate(config)
+  end
+
+  test "errors on bad template format" do
+    templates = %{"foo" => "Some template"}
+    config = put_in(minimal_config, ["templates"], templates)
+    response = validate(config)
+    assert {:error, [{"Type mismatch. Expected Object but got String.", "#/templates/foo"}], []} == response
+  end
+
+  test "templates must have proper names" do
+    templates = %{"1?" => %{"body" => "Some template"}}
+    config = put_in(minimal_config, ["templates"], templates)
+    response = validate(config)
+
+    # Not the greatest error message for this, sadly :(
+    assert response == {:error, [{"Schema does not allow additional properties.", "#/templates/1?"}], []}
+  end
+
+end

From 58993516dd1962f0dc0697a1a9ac133a1ded33d5 Mon Sep 17 00:00:00 2001
From: vanstee <patrick@vanstee.me>
Date: Tue, 6 Sep 2016 14:23:48 -0400
Subject: [PATCH 2/2] Include fields used for structured docs

---
 priv/schemas/bundle_config_schema_v4.yaml | 17 ++++++++++++++++-
 test/spanner/config/v4_validator_test.exs |  2 ++
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/priv/schemas/bundle_config_schema_v4.yaml b/priv/schemas/bundle_config_schema_v4.yaml
index 414eb0f..0d6959d 100644
--- a/priv/schemas/bundle_config_schema_v4.yaml
+++ b/priv/schemas/bundle_config_schema_v4.yaml
@@ -6,6 +6,7 @@ type: object
 required:
   - cog_bundle_version
   - name
+  - description
   - version
   - commands
 properties:
@@ -17,6 +18,12 @@ properties:
     type: string
   description:
     type: string
+  long_descirption:
+    type: string
+  author:
+    type: string
+  homepage:
+    type: string
   version:
     type: string
     pattern: ^\d+\.\d+($|\.\d+$)
@@ -71,8 +78,16 @@ definitions:
         type: string
       description:
         type: string
-      documentation:
+      long_description:
+        type: string
+      examples:
         type: string
+      arguments:
+        type: string
+      subcommands:
+        type: object
+        additionalProperties:
+          type: string
       rules:
         type: array
         items:
diff --git a/test/spanner/config/v4_validator_test.exs b/test/spanner/config/v4_validator_test.exs
index 7295624..be18241 100644
--- a/test/spanner/config/v4_validator_test.exs
+++ b/test/spanner/config/v4_validator_test.exs
@@ -19,6 +19,7 @@ defmodule Spanner.Config.V4ValidatorTest do
     %{"cog_bundle_version" => 4,
       "name" => "foo",
       "version" => "0.0.1",
+      "description" => "Does some foo and a bar",
       "commands" => %{
         "date" => %{
           "executable" => "/bin/date",
@@ -34,6 +35,7 @@ defmodule Spanner.Config.V4ValidatorTest do
     %{"cog_bundle_version" => 4,
       "name" => "foo",
       "version" => "0.1",
+      "description" => "Does some foo and a bar",
       "permissions" => ["foo:view"],
       "commands" => %{
         "bar" => %{"executable" => "/bin/bar",