Config, validation and test

plugins/providers/docker/config.rb

@@ -157,6 +157,7 @@ module VagrantPlugins
       def initialize
         @build_args = []
         @build_dir  = UNSET_VALUE
+        @git_repo   = UNSET_VALUE
         @cmd        = UNSET_VALUE
         @compose    = UNSET_VALUE
         @compose_configuration = {}
@@ -193,14 +194,39 @@ module VagrantPlugins
         super.tap do |result|
           # This is a bit confusing. The tests explain the purpose of this
           # better than the code lets on, I believe.
-          if (other.image != UNSET_VALUE || other.build_dir != UNSET_VALUE) &&
+          has_image     = (other.image != UNSET_VALUE)
-            (other.image == UNSET_VALUE || other.build_dir == UNSET_VALUE)
+          has_build_dir = (other.build_dir != UNSET_VALUE)
-            if other.image != UNSET_VALUE && @build_dir != UNSET_VALUE
+          has_git_repo  = (other.git_repo != UNSET_VALUE)
-              result.build_dir = nil
+          
+          if (has_image ^ has_build_dir ^ has_git_repo) && !(has_image && has_build_dir && has_git_repo)
+            # image
+            if has_image
+              if @build_dir != UNSET_VALUE
+                result.build_dir = nil
+              end
+              if @git_repo != UNSET_VALUE
+                result.git_repo = nil
+              end
             end
-
+            
-            if other.build_dir != UNSET_VALUE && @image != UNSET_VALUE
+            # build_dir
-              result.image = nil
+            if has_build_dir
+              if @image != UNSET_VALUE
+                result.image = nil
+              end
+              if @git_repo != UNSET_VALUE
+                result.git_repo = nil
+              end
+            end
+            
+            # git_repo
+            if has_git_repo
+              if @build_dir != UNSET_VALUE
+                result.build_dir = nil
+              end
+              if @image != UNSET_VALUE
+                result.image = nil
+              end
             end
           end
 
@@ -222,6 +248,7 @@ module VagrantPlugins
       def finalize!
         @build_args = [] if @build_args == UNSET_VALUE
         @build_dir  = nil if @build_dir == UNSET_VALUE
+        @git_repo   = nil if @git_repo == UNSET_VALUE
         @cmd        = [] if @cmd == UNSET_VALUE
         @compose    = false if @compose == UNSET_VALUE
         @create_args = [] if @create_args == UNSET_VALUE
@@ -269,12 +296,13 @@ module VagrantPlugins
 
       def validate(machine)
         errors = _detected_errors
-
+        
-        if @build_dir && @image
+        # FIXME: is there a more ruby-elegant way to write this?
+        if (@build_dir? 1 : 0) + (@git_repo? 1 : 0) + (@image? 1 : 0) > 1 
           errors << I18n.t("docker_provider.errors.config.both_build_and_image")
         end
 
-        if !@build_dir && !@image
+        if !@build_dir && !@git_repo && !@image
           errors << I18n.t("docker_provider.errors.config.build_dir_or_image")
         end
 
@@ -284,6 +312,11 @@ module VagrantPlugins
             errors << I18n.t("docker_provider.errors.config.build_dir_invalid")
           end
         end
+        
+        # Comparison logic taken directly from docker's urlutil.go
+        if @git_repo && !( @git_repo =~ /^http(?:s)?:\/\/.*.git(?:#.+)?$/ || @git_repo =~ /^git(?:hub\.com|@|:\/\/)/)
+          errors << I18n.t("docker_provider.errors.config.git_repo_invalid")
+        end
 
         if !@compose_configuration.is_a?(Hash)
           errors << I18n.t("docker_provider.errors.config.compose_configuration_hash")

templates/locales/providers_docker.yml

@@ -141,6 +141,8 @@ en:
           Only one of "build_dir", "git_repo" or "image" can be set
         build_dir_invalid: |-
           "build_dir" must exist and contain a Dockerfile
+        git_repo_invalid: |-
+          "git_repo" must be a valid repository URL
         build_dir_or_image: |-
           One of "build_dir", "git_repo" or "image" must be set
         compose_configuration_hash: |-

test/unit/plugins/providers/docker/config_test.rb

@@ -43,6 +43,7 @@ describe VagrantPlugins::DockerProvider::Config do
     before { subject.finalize! }
 
     its(:build_dir) { should be_nil }
+    its(:git_repo) { should be_nil }
     its(:expose) { should eq([]) }
     its(:cmd) { should eq([]) }
     its(:env) { should eq({}) }
@@ -67,16 +68,44 @@ describe VagrantPlugins::DockerProvider::Config do
     allow(Vagrant::Util::Platform).to receive(:linux?).and_return(true)
   end
 
-  it "should be invalid if both build dir and image are set" do
+  describe "should be invalid if any two or more of build dir, git repo and image are set" do
-    subject.build_dir = build_dir
+    it "build dir and image" do
-    subject.image = "foo"
+      subject.build_dir = build_dir
-    subject.finalize!
+      subject.image = "foo"
-    assert_invalid
+      subject.git_repo = nil
+      subject.finalize!
+      assert_invalid
+    end
+    
+    it "build dir and git repo" do
+      subject.build_dir = build_dir
+      subject.git_repo = "http://someone.com/something.git#branch:dir"
+      subject.image = nil
+      subject.finalize!
+      assert_invalid
+    end
+    
+    it "git repo dir and image" do
+      subject.build_dir = nil
+      subject.git_repo = "http://someone.com/something.git#branch:dir"
+      subject.image = "foo"
+      subject.finalize!
+      assert_invalid
+    end
+    
+    it "build dir, git repo and image" do
+      subject.build_dir = build_dir
+      subject.git_repo = "http://someone.com/something.git#branch:dir"
+      subject.image = "foo"
+      subject.finalize!
+      assert_invalid
+    end
   end
 
   describe "#build_dir" do
-    it "should be valid if not set with image" do
+    it "should be valid if not set with image or git repo" do
       subject.build_dir = nil
+      subject.git_repo = nil
       subject.image = "foo"
       subject.finalize!
       assert_valid
@@ -88,6 +117,52 @@ describe VagrantPlugins::DockerProvider::Config do
       assert_valid
     end
   end
+  
+  describe "#git_repo" do
+    it "should be valid if not set with image or build dir" do
+      subject.build_dir = nil
+      subject.git_repo = "http://someone.com/something.git#branch:dir"
+      subject.image = nil
+      subject.finalize!
+      assert_valid
+    end
+
+    it "should be valid with a http git url" do
+      subject.git_repo = "http://someone.com/something.git#branch:dir"
+      subject.finalize!
+      assert_valid
+    end
+    
+    it "should be valid with a git@ url" do
+      subject.git_repo = "git@someone.com:somebody/something"
+      subject.finalize!
+      assert_valid
+    end
+    
+    it "should be valid with a git:// url" do
+      subject.git_repo = "git://someone.com/something"
+      subject.finalize!
+      assert_valid
+    end
+    
+    it "should be valid with a short url beginning with github.com url" do
+      subject.git_repo = "github.com/somebody/something"
+      subject.finalize!
+      assert_valid
+    end
+    
+    it "should be invalid with an non-git url" do
+      subject.git_repo = "http://foo.bar.com"
+      subject.finalize!
+      assert_invalid
+    end
+    
+    it "should be invalid with an non url" do
+      subject.git_repo = "http||://foo.bar.com sdfs"
+      subject.finalize!
+      assert_invalid
+    end
+  end
 
   describe "#compose" do
     before do
@@ -179,7 +254,7 @@ describe VagrantPlugins::DockerProvider::Config do
 
     subject { one.merge(two) }
 
-    context "#build_dir and #image" do
+    context "#build_dir, #git_repo and #image" do
       it "overrides image if build_dir is set previously" do
         one.build_dir = "foo"
         two.image = "bar"
@@ -187,22 +262,72 @@ describe VagrantPlugins::DockerProvider::Config do
         expect(subject.build_dir).to be_nil
         expect(subject.image).to eq("bar")
       end
+      
+      it "overrides image if git_repo is set previously" do
+        one.git_repo = "foo"
+        two.image = "bar"
 
-      it "overrides image if build_dir is set previously" do
+        expect(subject.image).to eq("bar")
+        expect(subject.git_repo).to be_nil
+      end
+
+      it "overrides build_dir if image is set previously" do
         one.image = "foo"
         two.build_dir = "bar"
 
-        expect(subject.image).to be_nil
         expect(subject.build_dir).to eq("bar")
+        expect(subject.image).to be_nil
+      end
+      
+      it "overrides build_dir if git_repo is set previously" do
+        one.git_repo = "foo"
+        two.build_dir = "bar"
+       
+        expect(subject.build_dir).to eq("bar")
+        expect(subject.git_repo).to be_nil
+      end
+      
+      it "overrides git_repo if build_dir is set previously" do
+        one.build_dir = "foo"
+        two.git_repo = "bar"
+
+        expect(subject.build_dir).to be_nil
+        expect(subject.git_repo).to eq("bar")
+      end
+      
+      it "overrides git_repo if image is set previously" do
+        one.image = "foo"
+        two.git_repo = "bar"
+
+        expect(subject.image).to be_nil
+        expect(subject.git_repo).to eq("bar")
       end
 
-      it "preserves if both set" do
+      it "preserves if both image and build_dir are set" do
         one.image = "foo"
         two.image = "baz"
         two.build_dir = "bar"
 
-        expect(subject.image).to eq("baz")
         expect(subject.build_dir).to eq("bar")
+        expect(subject.image).to eq("baz")
+      end
+      
+      it "preserves if both image and git_repo are set" do
+        one.image = "foo"
+        two.image = "baz"
+        two.git_repo = "bar"
+
+        expect(subject.image).to eq("baz")
+        expect(subject.git_repo).to eq("bar")
+      end
+      
+      it "preserves if both build_dir and git_repo are set" do
+        one.build_dir = "foo"
+        two.build_dir = "baz"
+        two.git_repo = "bar"
+
+        expect(subject.build_dir).to eq("baz")
+        expect(subject.git_repo).to eq("bar")
       end
     end