From 64345034687bbcabd34b2036ff011d57f826c8b7 Mon Sep 17 00:00:00 2001 From: Orien Madgwick <497874+orien@users.noreply.github.com> Date: Wed, 13 Nov 2024 07:50:22 +1100 Subject: [PATCH] Reduce customisations --- Dockerfile | 2 -- app/models/git_repository.rb | 8 ++--- app/models/job_execution.rb | 1 - lib/samson/command_executor.rb | 21 +++---------- .../controllers/gke_clusters_controller.rb | 6 ++-- .../gcloud/lib/samson_gcloud/image_builder.rb | 6 ++-- .../gcloud/lib/samson_gcloud/image_tagger.rb | 8 ++--- .../gcloud/lib/samson_gcloud/tag_resolver.rb | 7 +++-- .../controllers/gcloud_controller_test.rb | 12 ++++---- .../gke_clusters_controller_test.rb | 4 +-- .../test/samson_gcloud/image_builder_test.rb | 4 +-- .../test/samson_gcloud/image_tagger_test.rb | 8 ++--- .../test/samson_gcloud/tag_resolver_test.rb | 6 ++-- test/integration/cleanliness_test.rb | 4 +++ test/lib/samson/command_executor_test.rb | 30 +++++++------------ test/models/git_repository_test.rb | 6 ++-- 16 files changed, 56 insertions(+), 77 deletions(-) diff --git a/Dockerfile b/Dockerfile index 4534b72599..a6639c2808 100644 --- a/Dockerfile +++ b/Dockerfile @@ -18,8 +18,6 @@ RUN \ npm && \ rm -rf /var/lib/apt/lists/* -RUN gem update --system 3.3.25 - RUN curl -fsSL https://get.docker.com | bash - WORKDIR /app diff --git a/app/models/git_repository.rb b/app/models/git_repository.rb index ebebd7632b..20074a4fa4 100644 --- a/app/models/git_repository.rb +++ b/app/models/git_repository.rb @@ -185,15 +185,13 @@ def instance_cache(key) # error: nil def capture_stdout(*command, dir: repo_cache_dir) ::Rails.logger.info("Running command #{command}") - result = Samson::CommandExecutor.execute( + success, output = Samson::CommandExecutor.execute( *command, whitelist_env: ['HOME', 'PATH'], timeout: 30.minutes, dir: dir ) - unless result.status - ::Rails.logger.error("Failed to run command #{command}: #{result.error}") - end - result.output.strip if result.status + ::Rails.logger.error("Failed to run command #{command}: #{error}") unless result.success + output.strip if result.success end end diff --git a/app/models/job_execution.rb b/app/models/job_execution.rb index e68cbce918..455d5c9c2c 100644 --- a/app/models/job_execution.rb +++ b/app/models/job_execution.rb @@ -181,7 +181,6 @@ def resolve_ref_to_commit(source) found end tag = @repository.fuzzy_tag_from_ref(source) - if commit @job.update_git_references!(commit: commit, tag: tag) diff --git a/lib/samson/command_executor.rb b/lib/samson/command_executor.rb index 4b5dba2711..8872a5f6a9 100644 --- a/lib/samson/command_executor.rb +++ b/lib/samson/command_executor.rb @@ -1,4 +1,5 @@ # frozen_string_literal: true + require 'tempfile' module Samson @@ -22,28 +23,14 @@ def execute(*command, timeout:, whitelist_env: [], env: {}, dir: nil) pio = IO.popen(env, command.map(&:to_s), popen_options) output = pio.read pio.close - result = OpenStruct.new( - output: output, - error: File.read(stderr), - status: $?.success? - ) - pio.close - result + [$?.success?, output, File.read(stderr)] rescue Errno::ENOENT - OpenStruct.new( - error: "No such file or directory - #{command.first}", - status: false, - output: "" - ) + [false, "", "No such file or directory - #{command.first}"] end end end rescue Timeout::Error - OpenStruct.new( - error: $!.message, - status: false, - output: "" - ) + [false, "", $!.message] ensure if pio && !pio.closed? kill_process pio.pid diff --git a/plugins/gcloud/app/controllers/gke_clusters_controller.rb b/plugins/gcloud/app/controllers/gke_clusters_controller.rb index ff41b4514a..5e0d5cea06 100644 --- a/plugins/gcloud/app/controllers/gke_clusters_controller.rb +++ b/plugins/gcloud/app/controllers/gke_clusters_controller.rb @@ -34,16 +34,16 @@ def create "gcloud", "container", "clusters", "get-credentials", "--zone", zone, cluster, *SamsonGcloud.cli_options(project: project) ] - result = Samson::CommandExecutor.execute( + success, content = Samson::CommandExecutor.execute( *command, whitelist_env: ["PATH"], timeout: 10, env: {"KUBECONFIG" => path, "CLOUDSDK_CONTAINER_USE_CLIENT_CERTIFICATE" => "True"} ) - unless result.status + unless success flash.now[:alert] = "Failed to execute (make sure container.cluster.getCredentials permissions are granted): " \ - "#{command.join(" ")} #{result.output}" + "#{command.join(" ")} #{content}" return render :new end diff --git a/plugins/gcloud/lib/samson_gcloud/image_builder.rb b/plugins/gcloud/lib/samson_gcloud/image_builder.rb index 8d57318bc4..f5bea6ef2b 100644 --- a/plugins/gcloud/lib/samson_gcloud/image_builder.rb +++ b/plugins/gcloud/lib/samson_gcloud/image_builder.rb @@ -88,12 +88,12 @@ def prevent_upload_of_ignored_files(dir, build) # NOTE: not using executor since it does not return output def image_exists_in_gcloud?(repo_digest) image, digest = repo_digest.split('@') - result = Samson::CommandExecutor.execute( + output = Samson::CommandExecutor.execute( "gcloud", "container", "images", "list-tags", image, "--format", "get(digest)", "--filter", "digest=#{digest}", *SamsonGcloud.cli_options, timeout: 10 - ) - result.output.strip == digest + ).last + output.strip == digest end end end diff --git a/plugins/gcloud/lib/samson_gcloud/image_tagger.rb b/plugins/gcloud/lib/samson_gcloud/image_tagger.rb index 5883fe4ebb..c3095e761c 100644 --- a/plugins/gcloud/lib/samson_gcloud/image_tagger.rb +++ b/plugins/gcloud/lib/samson_gcloud/image_tagger.rb @@ -33,14 +33,14 @@ def tag_image(tag, base, digest, job_output) command = [ "gcloud", "container", "images", "add-tag", digest, "#{base}:#{tag}", "--quiet", *SamsonGcloud.cli_options ] - result = Samson::CommandExecutor.execute(*command, timeout: 10, whitelist_env: ["PATH"]) + success, output = Samson::CommandExecutor.execute(*command, timeout: 10, whitelist_env: ["PATH"]) job_output.write <<~TEXT #{Samson::OutputUtils.timestamp} Tagging GCR image: #{command.join(" ")} - #{result.output.strip} + #{output.strip} TEXT - job_output.puts "FAILED" unless result.status - result.status + job_output.puts "FAILED" unless success + success end def cache_last_tagged(key, value) diff --git a/plugins/gcloud/lib/samson_gcloud/tag_resolver.rb b/plugins/gcloud/lib/samson_gcloud/tag_resolver.rb index cd8317ce0e..8c3c74ba81 100644 --- a/plugins/gcloud/lib/samson_gcloud/tag_resolver.rb +++ b/plugins/gcloud/lib/samson_gcloud/tag_resolver.rb @@ -10,14 +10,15 @@ def resolve_docker_image_tag(image) return unless SamsonGcloud.gcr? image return if image.match? Build::DIGEST_REGEX - result = Samson::CommandExecutor.execute( + success, json = Samson::CommandExecutor.execute( "gcloud", "container", "images", "describe", image, "--format", "json", *SamsonGcloud.cli_options, + err: '/dev/null', timeout: 10, whitelist_env: ["PATH"] ) - raise "GCLOUD ERROR: unable to resolve #{image}\n#{result.error}" unless result.status - digest = JSON.parse(result.output).dig_fetch("image_summary", "digest") + raise "GCLOUD ERROR: unable to resolve #{image}\n#{json}" unless success + digest = JSON.parse(json).dig_fetch("image_summary", "digest") base = image.split(":", 2).first "#{base}@#{digest}" diff --git a/plugins/gcloud/test/controllers/gcloud_controller_test.rb b/plugins/gcloud/test/controllers/gcloud_controller_test.rb index 37c44f214a..b6f8591905 100644 --- a/plugins/gcloud/test/controllers/gcloud_controller_test.rb +++ b/plugins/gcloud/test/controllers/gcloud_controller_test.rb @@ -30,7 +30,7 @@ def do_sync end it "can sync" do - Samson::CommandExecutor.expects(:execute).returns(OpenStruct.new(status: true, output: result.to_json)) + Samson::CommandExecutor.expects(:execute).returns([true, result.to_json, ""]) do_sync assert flash[:notice] build.reload @@ -40,13 +40,13 @@ def do_sync it "can sync images with a tag" do result[:results][:images][1][:name] += ":foo" - Samson::CommandExecutor.expects(:execute).returns(OpenStruct.new(status: true, output: result.to_json)) + Samson::CommandExecutor.expects(:execute).returns([true, result.to_json, ""]) do_sync build.reload.docker_repo_digest.must_equal repo_digest end it "fails when gcloud cli fails" do - Samson::CommandExecutor.expects(:execute).returns(OpenStruct.new(status: false, output: result.to_json)) + Samson::CommandExecutor.expects(:execute).returns([false, result.to_json, ""]) do_sync assert flash[:alert] end @@ -55,7 +55,7 @@ def do_sync let(:image_name) { "gcr.io/foo*baz+bing/#{build.image_name}" } it "fails when digest does not pass validations" do - Samson::CommandExecutor.expects(:execute).returns(OpenStruct.new(status: true, output: result.to_json)) + Samson::CommandExecutor.expects(:execute).returns([true, result.to_json, ""]) do_sync @@ -66,7 +66,7 @@ def do_sync it "fails when image is not found" do result[:results][:images].last[:name] = "gcr.io/other" - Samson::CommandExecutor.expects(:execute).returns(OpenStruct.new(status: true, output: result.to_json)) + Samson::CommandExecutor.expects(:execute).returns([true, result.to_json, ""]) do_sync @@ -77,7 +77,7 @@ def do_sync it "can store failures" do result[:status] = "QUEUED" result.delete(:results) - Samson::CommandExecutor.expects(:execute).returns(OpenStruct.new(status: true, output: result.to_json)) + Samson::CommandExecutor.expects(:execute).returns([true, result.to_json, ""]) do_sync diff --git a/plugins/gcloud/test/controllers/gke_clusters_controller_test.rb b/plugins/gcloud/test/controllers/gke_clusters_controller_test.rb index 7b997eb0d4..72b3a96016 100644 --- a/plugins/gcloud/test/controllers/gke_clusters_controller_test.rb +++ b/plugins/gcloud/test/controllers/gke_clusters_controller_test.rb @@ -38,7 +38,7 @@ def do_create(gcp_project: 'pp', cluster_name: 'cc', zone: 'zz') timeout: 10, env: {'KUBECONFIG' => expected_file, "CLOUDSDK_CONTAINER_USE_CLIENT_CERTIFICATE" => "True"}, whitelist_env: ['PATH'] - ).returns(OpenStruct.new(status: true, output: "foo")) + ).returns([true, "foo", ""]) do_create assert_redirected_to( "/kubernetes/clusters/new?kubernetes_cluster%5Bconfig_filepath%5D=#{CGI.escape(expected_file)}" @@ -48,7 +48,7 @@ def do_create(gcp_project: 'pp', cluster_name: 'cc', zone: 'zz') end it "shows errors when command fails" do - Samson::CommandExecutor.expects(:execute).returns(OpenStruct.new(status: false, output: "foo")) + Samson::CommandExecutor.expects(:execute).returns([false, "foo", ""]) do_create assert_response :success flash[:alert].must_include( diff --git a/plugins/gcloud/test/samson_gcloud/image_builder_test.rb b/plugins/gcloud/test/samson_gcloud/image_builder_test.rb index 88779f60f1..487ef8eb20 100644 --- a/plugins/gcloud/test/samson_gcloud/image_builder_test.rb +++ b/plugins/gcloud/test/samson_gcloud/image_builder_test.rb @@ -46,7 +46,7 @@ def build_image(tag_as_latest: false, cache_from: nil) end it "can use cache" do - Samson::CommandExecutor.expects(:execute).returns(OpenStruct.new(status: true, output: "sha256:abc\n")) + Samson::CommandExecutor.expects(:execute).returns([true, "sha256:abc\n", ""]) old = 'gcr.io/something-old@sha256:abc' build_image(cache_from: old) File.read("some-dir/cloudbuild.yml").must_equal <<~YML @@ -63,7 +63,7 @@ def build_image(tag_as_latest: false, cache_from: nil) end it "does not use cache when image is not available" do - Samson::CommandExecutor.expects(:execute).returns(OpenStruct.new(status: true, output: "\n")) + Samson::CommandExecutor.expects(:execute).returns([true, "\n", ""]) build_image(cache_from: 'gcr.io/something-old') File.read("some-dir/cloudbuild.yml").must_equal <<~YML steps: diff --git a/plugins/gcloud/test/samson_gcloud/image_tagger_test.rb b/plugins/gcloud/test/samson_gcloud/image_tagger_test.rb index 8cd3d31d08..14e5797db7 100644 --- a/plugins/gcloud/test/samson_gcloud/image_tagger_test.rb +++ b/plugins/gcloud/test/samson_gcloud/image_tagger_test.rb @@ -16,7 +16,7 @@ def assert_tagged_with(tag, opts: []) Samson::CommandExecutor.expects(:execute).with( 'gcloud', 'container', 'images', 'add-tag', 'gcr.io/sdfsfsdf@some-sha', "gcr.io/sdfsfsdf:#{tag}", '--quiet', *opts, *auth_options, anything - ).returns(OpenStruct.new(status: true, output: "OUT")) + ).returns([true, "OUT", ""]) end with_env DOCKER_FEATURE: 'true', GCLOUD_PROJECT: '123', GCLOUD_ACCOUNT: 'acc' @@ -53,7 +53,7 @@ def assert_tagged_with(tag, opts: []) end it "tries again when tagging failed" do - Samson::CommandExecutor.expects(:execute).returns(OpenStruct.new(status: false, output: 'x')).times(2) + Samson::CommandExecutor.expects(:execute).returns([false, 'x', ""]).times(2) 2.times { tag } end @@ -72,7 +72,7 @@ def assert_tagged_with(tag, opts: []) it "tags other regions" do build.update_column(:docker_repo_digest, 'asia.gcr.io/sdfsfsdf@some-sha') - Samson::CommandExecutor.expects(:execute).returns(OpenStruct.new(status: true, output: "OUT")) + Samson::CommandExecutor.expects(:execute).returns([true, "OUT", ""]) tag end @@ -89,7 +89,7 @@ def assert_tagged_with(tag, opts: []) end it "shows tagging errors" do - Samson::CommandExecutor.expects(:execute).returns(OpenStruct.new(status: false, output: "NOPE")) + Samson::CommandExecutor.expects(:execute).returns([false, "NOPE", ""]) tag output_serialized.must_include "NOPE" end diff --git a/plugins/gcloud/test/samson_gcloud/tag_resolver_test.rb b/plugins/gcloud/test/samson_gcloud/tag_resolver_test.rb index 8621288e69..3102ecb0c8 100644 --- a/plugins/gcloud/test/samson_gcloud/tag_resolver_test.rb +++ b/plugins/gcloud/test/samson_gcloud/tag_resolver_test.rb @@ -12,13 +12,13 @@ it "resolves latest" do Samson::CommandExecutor.expects(:execute). - returns(OpenStruct.new(status: true, output: {image_summary: {digest: digest}}.to_json)) + returns([true, {image_summary: {digest: digest}}.to_json, ""]) SamsonGcloud::TagResolver.resolve_docker_image_tag(image).must_equal "#{image}@#{digest}" end it "resolves custom tag" do Samson::CommandExecutor.expects(:execute). - returns(OpenStruct.new(status: true, output: {image_summary: {digest: digest}}.to_json)) + returns([true, {image_summary: {digest: digest}}.to_json, ""]) SamsonGcloud::TagResolver.resolve_docker_image_tag("#{image}:foo").must_equal "#{image}@#{digest}" end @@ -32,7 +32,7 @@ it "raises on gcloud error" do Samson::CommandExecutor.expects(:execute). - returns(OpenStruct.new(status: false, output: "")) + returns([false, "", ""]) assert_raises RuntimeError do SamsonGcloud::TagResolver.resolve_docker_image_tag(image) end diff --git a/test/integration/cleanliness_test.rb b/test/integration/cleanliness_test.rb index f6cccaf1ee..779596c809 100644 --- a/test/integration/cleanliness_test.rb +++ b/test/integration/cleanliness_test.rb @@ -236,6 +236,10 @@ def assert_content(files) values.uniq.size.must_equal 1, "Expected all places to use consistent PERIODICAL value, but found #{values.inspect}" end + it "has gitignore and dockerignore in sync" do + File.read(".dockerignore").must_include File.read(".gitignore") + end + it "explicity defines what should happen to dependencies" do bad = all_models.flat_map do |model| model.reflect_on_all_associations.map do |association| diff --git a/test/lib/samson/command_executor_test.rb b/test/lib/samson/command_executor_test.rb index 09a4d3f244..17f2deb735 100644 --- a/test/lib/samson/command_executor_test.rb +++ b/test/lib/samson/command_executor_test.rb @@ -6,29 +6,25 @@ describe Samson::CommandExecutor do describe "#execute" do it "runs" do - Samson::CommandExecutor.execute("echo", "hello", timeout: 1). - must_equal OpenStruct.new(status: true, error: "", output: "hello\n") + Samson::CommandExecutor.execute("echo", "hello", timeout: 1).must_equal [true, "hello\n", ""] end it "captures stderr" do - Samson::CommandExecutor.execute("sh", "-c", "echo hello 1>&2", timeout: 1). - must_equal OpenStruct.new(status: true, error: "hello\n", output: "") + Samson::CommandExecutor.execute("sh", "-c", "echo hello 1>&2", timeout: 1).must_equal [true, "hello\n", ""] end it "runs in specified directory" do Dir.mktmpdir("foobar") do |dir| - Samson::CommandExecutor.execute("pwd", timeout: 1, dir: dir).output.must_include dir + Samson::CommandExecutor.execute("pwd", timeout: 1, dir: dir).second.must_include dir end end it "fails nicely on missing exectable" do - Samson::CommandExecutor.execute("foo", "bar", timeout: 1). - must_equal OpenStruct.new(status: false, error: "No such file or directory - foo", output: "") + Samson::CommandExecutor.execute("foo", "bar", timeout: 1).must_equal [false, "No such file or directory - foo", ""] end it "does not fail on nil commands" do - Samson::CommandExecutor.execute("echo", 1, nil, timeout: 1). - must_equal OpenStruct.new(status: true, error: "", output: "1 \n") + Samson::CommandExecutor.execute("echo", 1, nil, timeout: 1).must_equal [true, "1 \n", ""] end it "shows full backtrace when failing" do @@ -43,8 +39,7 @@ command = ["sleep", "15"] Samson::CommandExecutor.expects(:sleep) # waiting after kill ... no need to make this test slow time = Benchmark.realtime do - Samson::CommandExecutor.execute(*command, timeout: 0.1). - must_equal OpenStruct.new(status: false, error: "execution expired", output: "") + Samson::CommandExecutor.execute(*command, timeout: 0.1).must_equal [false, "", "execution expired"] end time.must_be :<, 0.2 `ps -ef`.wont_include(command.join(" ")) @@ -52,8 +47,7 @@ it "does not fail when pid was already gone" do Process.expects(:kill).raises(Errno::ESRCH) # simulate that pid was gone and kill failed - Samson::CommandExecutor.execute("sleep", "0.2", timeout: 0.1). - must_equal OpenStruct.new(status: false, error: "execution expired", output: "") + Samson::CommandExecutor.execute("sleep", "0.2", timeout: 0.1).must_equal [false, "", "execution expired"] sleep 0.2 # do not leave process thread hanging end @@ -61,8 +55,7 @@ Samson::CommandExecutor.expects(:sleep) # waiting after kill ... we ignore it in this test Process.expects(:kill).twice # simulate that process could not be killed with :KILL time = Benchmark.realtime do - Samson::CommandExecutor.execute("sleep", "0.5", timeout: 0.1). - must_equal OpenStruct.new(status: false, error: "execution expired", output: "") + Samson::CommandExecutor.execute("sleep", "0.5", timeout: 0.1).must_equal [false, "", "execution expired"] end time.must_be :>, 0.5 end @@ -80,20 +73,19 @@ it "does not allow env access" do with_env FOO: 'bar' do - Samson::CommandExecutor.execute("printenv", "FOO", timeout: 1). - must_equal OpenStruct.new(status: false, error: "", output: "") + Samson::CommandExecutor.execute("printenv", "FOO", timeout: 1).must_equal [false, "", ""] end end it "can set env" do Samson::CommandExecutor.execute("printenv", "FOO", timeout: 1, env: {"FOO" => "baz"}). - must_equal OpenStruct.new(status: true, error: "", output: "baz\n") + must_equal [true, "baz\n", ""] end it "allows whitelisted env access" do with_env FOO: 'bar' do Samson::CommandExecutor.execute("printenv", "FOO", timeout: 1, whitelist_env: ["FOO"]). - must_equal OpenStruct.new(status: true, error: "", output: "bar\n") + must_equal [true, "bar\n", ""] end end end diff --git a/test/models/git_repository_test.rb b/test/models/git_repository_test.rb index 384fcd6bcd..f48d829e03 100644 --- a/test/models/git_repository_test.rb +++ b/test/models/git_repository_test.rb @@ -310,12 +310,12 @@ def call end it "caches" do - Samson::CommandExecutor.expects(:execute).times(2).returns(OpenStruct.new(status: true, output: "x")) + Samson::CommandExecutor.expects(:execute).times(2).returns([true, "x", ""]) 4.times { repository.file_content('foo', sha).must_equal "x" } end it "caches sha too" do - Samson::CommandExecutor.expects(:execute).times(3).returns(OpenStruct.new(status: true, output: "x")) + Samson::CommandExecutor.expects(:execute).times(3).returns([true, "x", ""]) 4.times { |i| repository.file_content("foo-#{i.odd?}", sha).must_equal "x" } end @@ -344,7 +344,7 @@ def call it "does not cache when requesting for an update" do repository.unstub(:ensure_mirror_current) repository.expects(:ensure_mirror_current) - Samson::CommandExecutor.expects(:execute).times(2).returns(OpenStruct.new(status: true, output: "x")) + Samson::CommandExecutor.expects(:execute).times(2).returns([true, "x", ""]) repository.file_content('foo', 'HEAD', pull: false).must_equal "x" 4.times { repository.file_content('foo', 'HEAD', pull: true).must_equal "x" } end