From de6a1794c75be4fadd3548728412b5df81019706 Mon Sep 17 00:00:00 2001 From: Oleksiy Protas Date: Mon, 17 Oct 2016 00:16:33 +0300 Subject: [PATCH] Config, validation and test --- plugins/providers/docker/config.rb | 53 +++++-- templates/locales/providers_docker.yml | 2 + .../plugins/providers/docker/config_test.rb | 147 ++++++++++++++++-- 3 files changed, 181 insertions(+), 21 deletions(-) diff --git a/plugins/providers/docker/config.rb b/plugins/providers/docker/config.rb index 7ad23908c..6abc997cc 100644 --- a/plugins/providers/docker/config.rb +++ b/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) && - (other.image == UNSET_VALUE || other.build_dir == UNSET_VALUE) - if other.image != UNSET_VALUE && @build_dir != UNSET_VALUE - result.build_dir = nil + has_image = (other.image != UNSET_VALUE) + has_build_dir = (other.build_dir != UNSET_VALUE) + has_git_repo = (other.git_repo != UNSET_VALUE) + + 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 - result.image = nil + + # build_dir + 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") diff --git a/templates/locales/providers_docker.yml b/templates/locales/providers_docker.yml index 3ad6820a7..df1fa2ceb 100644 --- a/templates/locales/providers_docker.yml +++ b/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: |- diff --git a/test/unit/plugins/providers/docker/config_test.rb b/test/unit/plugins/providers/docker/config_test.rb index 063634872..ba9a0088a 100644 --- a/test/unit/plugins/providers/docker/config_test.rb +++ b/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 - subject.build_dir = build_dir - subject.image = "foo" - subject.finalize! - assert_invalid + describe "should be invalid if any two or more of build dir, git repo and image are set" do + it "build dir and image" do + subject.build_dir = build_dir + subject.image = "foo" + 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