From e103d3f172d05f40fce01550ff58d10eced53e86 Mon Sep 17 00:00:00 2001 From: Jamie Magee Date: Sun, 7 Jan 2024 21:48:42 -0800 Subject: [PATCH 1/4] Strong type `Dependabot::PullRequestCreator` --- common/lib/dependabot/pull_request_creator.rb | 168 ++++++++++++++++-- 1 file changed, 151 insertions(+), 17 deletions(-) diff --git a/common/lib/dependabot/pull_request_creator.rb b/common/lib/dependabot/pull_request_creator.rb index 4ea04440a7b..a568dcc3e1f 100644 --- a/common/lib/dependabot/pull_request_creator.rb +++ b/common/lib/dependabot/pull_request_creator.rb @@ -1,10 +1,13 @@ -# typed: true +# typed: strong # frozen_string_literal: true +require "sorbet-runtime" require "dependabot/metadata_finders" module Dependabot class PullRequestCreator + extend T::Sig + require "dependabot/pull_request_creator/azure" require "dependabot/pull_request_creator/bitbucket" require "dependabot/pull_request_creator/codecommit" @@ -42,7 +45,18 @@ class UnexpectedError < StandardError; end # AnnotationError is raised if a PR was created, but failed annotation class AnnotationError < StandardError - attr_reader :cause, :pull_request + extend T::Sig + + sig { returns(StandardError) } + attr_reader :cause + + # TODO: Currently, this error is only used by the GitHub PR creator. + # An Octokit update will likely give this a proper type, + # but we should consider a `Dependabot::PullRequest` type. + sig { returns(Sawyer::Resource) } + attr_reader :pull_request + + sig { params(cause: StandardError, pull_request: Sawyer::Resource).void } def initialize(cause, pull_request) super(cause.message) @cause = cause @@ -50,15 +64,113 @@ def initialize(cause, pull_request) end end - attr_reader :source, :dependencies, :files, :base_commit, - :credentials, :pr_message_header, :pr_message_footer, - :custom_labels, :author_details, :signature_key, - :commit_message_options, :vulnerabilities_fixed, - :reviewers, :assignees, :milestone, :branch_name_separator, - :branch_name_prefix, :branch_name_max_length, :github_redirection_service, - :custom_headers, :provider_metadata, :dependency_group, :pr_message_max_length, - :pr_message_encoding - + sig { returns(Dependabot::Source) } + attr_reader :source + + sig { returns(T::Array[Dependabot::Dependency]) } + attr_reader :dependencies + + sig { returns(T::Array[Dependabot::DependencyFile]) } + attr_reader :files + + sig { returns(String) } + attr_reader :base_commit + + sig { returns(T::Array[T::Hash[String, String]]) } + attr_reader :credentials + + sig { returns(T.nilable(String)) } + attr_reader :pr_message_header + + sig { returns(T.nilable(String)) } + attr_reader :pr_message_footer + + sig { returns(T.nilable(T::Array[String])) } + attr_reader :custom_labels + + sig { returns(T.nilable(T::Hash[Symbol, String])) } + attr_reader :author_details + + sig { returns(T.nilable(String)) } + attr_reader :signature_key + + sig { returns(T::Hash[Symbol, T.untyped]) } + attr_reader :commit_message_options + + sig { returns(T::Hash[String, String]) } + attr_reader :vulnerabilities_fixed + + sig { returns(T.nilable(T::Array[String])) } + attr_reader :reviewers + + sig { returns(T.nilable(T::Array[String])) } + attr_reader :assignees + + sig { returns(T.nilable(String)) } + attr_reader :milestone + + sig { returns(String) } + attr_reader :branch_name_separator + + sig { returns(String) } + attr_reader :branch_name_prefix + + sig { returns(T.nilable(Integer)) } + attr_reader :branch_name_max_length + + sig { returns(String) } + attr_reader :github_redirection_service + + sig { returns(T.nilable(T::Hash[String, String])) } + attr_reader :custom_headers + + sig { returns(T.nilable(T::Hash[Symbol, T.untyped])) } + attr_reader :provider_metadata + + sig { returns(T.nilable(Dependabot::DependencyGroup)) } + attr_reader :dependency_group + + sig { returns(T.nilable(Integer)) } + attr_reader :pr_message_max_length + + sig { returns(T.nilable(Encoding)) } + attr_reader :pr_message_encoding + + sig do + params( + source: Dependabot::Source, + base_commit: String, + dependencies: T::Array[Dependabot::Dependency], + files: T::Array[Dependabot::DependencyFile], + credentials: T::Array[T::Hash[String, String]], + pr_message_header: T.nilable(String), + pr_message_footer: T.nilable(String), + custom_labels: T.nilable(T::Array[String]), + author_details: T.nilable(T::Hash[Symbol, String]), + signature_key: T.nilable(String), + commit_message_options: T::Hash[Symbol, T.untyped], + vulnerabilities_fixed: T::Hash[String, String], + reviewers: T.nilable(T::Array[String]), + assignees: T.nilable(T::Array[String]), + milestone: T.nilable(String), + branch_name_separator: String, + branch_name_prefix: String, + branch_name_max_length: T.nilable(Integer), + label_language: T::Boolean, + automerge_candidate: T::Boolean, + github_redirection_service: String, + custom_headers: T.nilable(T::Hash[String, String]), + require_up_to_date_base: T::Boolean, + provider_metadata: T.nilable(T::Hash[Symbol, T.untyped]), + message: T.nilable( + T.any(Dependabot::PullRequestCreator::Message, Dependabot::PullRequestCreator::MessageBuilder) + ), + dependency_group: T.nilable(Dependabot::DependencyGroup), + pr_message_max_length: T.nilable(Integer), + pr_message_encoding: T.nilable(Encoding) + ) + .void + end def initialize(source:, base_commit:, dependencies:, files:, credentials:, pr_message_header: nil, pr_message_footer: nil, custom_labels: nil, author_details: nil, signature_key: nil, @@ -103,6 +215,7 @@ def initialize(source:, base_commit:, dependencies:, files:, credentials:, check_dependencies_have_previous_version end + sig { void } def check_dependencies_have_previous_version return if dependencies.all? { |d| requirements_changed?(d) } return if dependencies.all?(&:previous_version) @@ -111,6 +224,10 @@ def check_dependencies_have_previous_version "requirement to have a pull request created for them!" end + # TODO: This returns client-specific objects. + # We should create a standard interface (`Dependabot::PullRequest`) and + # then convert to that + sig { returns(T.untyped) } def create case source.provider when "github" then github_creator.create @@ -124,18 +241,22 @@ def create private + sig { returns(T::Boolean) } def label_language? @label_language end + sig { returns(T::Boolean) } def automerge_candidate? @automerge_candidate end + sig { returns(T::Boolean) } def require_up_to_date_base? @require_up_to_date_base end + sig { returns(Dependabot::PullRequestCreator::Github) } def github_creator Github.new( source: source, @@ -157,6 +278,7 @@ def github_creator ) end + sig { returns(Dependabot::PullRequestCreator::Gitlab) } def gitlab_creator Gitlab.new( source: source, @@ -172,10 +294,11 @@ def gitlab_creator approvers: reviewers, assignees: assignees, milestone: milestone, - target_project_id: provider_metadata[:target_project_id] + target_project_id: provider_metadata&.fetch(:target_project_id) ) end + sig { returns(Dependabot::PullRequestCreator::Azure) } def azure_creator Azure.new( source: source, @@ -194,6 +317,7 @@ def azure_creator ) end + sig { returns(Dependabot::PullRequestCreator::Bitbucket) } def bitbucket_creator Bitbucket.new( source: source, @@ -210,6 +334,7 @@ def bitbucket_creator ) end + sig { returns(Dependabot::PullRequestCreator::Codecommit) } def codecommit_creator Codecommit.new( source: source, @@ -226,6 +351,7 @@ def codecommit_creator ) end + sig { returns(T.any(Dependabot::PullRequestCreator::Message, Dependabot::PullRequestCreator::MessageBuilder)) } def message return @message unless @message.nil? @@ -257,8 +383,9 @@ def message ) end + sig { returns(Dependabot::PullRequestCreator::BranchNamer) } def branch_namer - @branch_namer ||= + @branch_namer ||= T.let( BranchNamer.new( dependencies: dependencies, files: files, @@ -268,11 +395,14 @@ def branch_namer prefix: branch_name_prefix, max_length: branch_name_max_length, includes_security_fixes: includes_security_fixes? - ) + ), + T.nilable(Dependabot::PullRequestCreator::BranchNamer) + ) end + sig { returns(Dependabot::PullRequestCreator::Labeler) } def labeler - @labeler ||= + @labeler ||= T.let( Labeler.new( source: source, custom_labels: custom_labels, @@ -281,15 +411,19 @@ def labeler dependencies: dependencies, label_language: label_language?, automerge_candidate: automerge_candidate? - ) + ), + T.nilable(Dependabot::PullRequestCreator::Labeler) + ) end + sig { returns(T::Boolean) } def includes_security_fixes? vulnerabilities_fixed.values.flatten.any? end + sig { params(dependency: Dependabot::Dependency).returns(T::Boolean) } def requirements_changed?(dependency) - (dependency.requirements - dependency.previous_requirements).any? + (dependency.requirements - T.must(dependency.previous_requirements)).any? end end end From 6badd20c813c224acebf65ba1460508eb1f5b6f1 Mon Sep 17 00:00:00 2001 From: Ryan Brandenburg Date: Tue, 9 Jan 2024 13:49:45 -0800 Subject: [PATCH 2/4] Handle unexpected kub image shapes --- docker/lib/dependabot/docker/file_parser.rb | 6 +++++- docker/spec/dependabot/docker/file_parser_spec.rb | 5 +++++ .../fixtures/kubernetes/yaml/unexpected_image.yaml | 10 ++++++++++ 3 files changed, 20 insertions(+), 1 deletion(-) create mode 100644 docker/spec/fixtures/kubernetes/yaml/unexpected_image.yaml diff --git a/docker/lib/dependabot/docker/file_parser.rb b/docker/lib/dependabot/docker/file_parser.rb index cd41ab9b726..7ad5ad4c638 100644 --- a/docker/lib/dependabot/docker/file_parser.rb +++ b/docker/lib/dependabot/docker/file_parser.rb @@ -111,7 +111,11 @@ def workfile_file_dependencies(file) images.each do |string| # TODO: Support Docker references and path references - details = string.match(IMAGE_SPEC).named_captures + match = string.match(IMAGE_SPEC) + + next if match.nil? + + details = match.named_captures details["registry"] = nil if details["registry"] == "docker.io" version = version_from(details) diff --git a/docker/spec/dependabot/docker/file_parser_spec.rb b/docker/spec/dependabot/docker/file_parser_spec.rb index 6391f60076c..d08457ec9a3 100644 --- a/docker/spec/dependabot/docker/file_parser_spec.rb +++ b/docker/spec/dependabot/docker/file_parser_spec.rb @@ -679,6 +679,11 @@ end end + context "with unknown tag" do + let(:podfile_fixture_name) { "unexpected_image.yaml" } + its(:length) { is_expected.to eq(0) } + end + context "with no tag or digest" do let(:podfile_fixture_name) { "bare.yaml" } its(:length) { is_expected.to eq(0) } diff --git a/docker/spec/fixtures/kubernetes/yaml/unexpected_image.yaml b/docker/spec/fixtures/kubernetes/yaml/unexpected_image.yaml new file mode 100644 index 00000000000..b2e1cdbaa4b --- /dev/null +++ b/docker/spec/fixtures/kubernetes/yaml/unexpected_image.yaml @@ -0,0 +1,10 @@ +apiVersion: v1 +kind: Pod +metadata: + name: ubuntu +spec: + containers: + - name: ubuntu + image: <>/sdf/official/stuff/thing:<> + ports: + - containerPort: 80 From e55f5d6746ed44ce666eec7d8b7e7b96a339c0d2 Mon Sep 17 00:00:00 2001 From: Ryan Brandenburg Date: Tue, 9 Jan 2024 15:25:35 -0800 Subject: [PATCH 3/4] Use safe navigation operator --- docker/lib/dependabot/docker/file_parser.rb | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/docker/lib/dependabot/docker/file_parser.rb b/docker/lib/dependabot/docker/file_parser.rb index 7ad5ad4c638..23168f3053f 100644 --- a/docker/lib/dependabot/docker/file_parser.rb +++ b/docker/lib/dependabot/docker/file_parser.rb @@ -111,11 +111,8 @@ def workfile_file_dependencies(file) images.each do |string| # TODO: Support Docker references and path references - match = string.match(IMAGE_SPEC) - - next if match.nil? - - details = match.named_captures + details = string.match(IMAGE_SPEC)&.named_captures + next if details.nil? details["registry"] = nil if details["registry"] == "docker.io" version = version_from(details) From cf55b5f4eb8baf7704595a5ef68200e9b924bc8e Mon Sep 17 00:00:00 2001 From: Ryan Brandenburg Date: Tue, 9 Jan 2024 15:32:23 -0800 Subject: [PATCH 4/4] Lint --- docker/lib/dependabot/docker/file_parser.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/docker/lib/dependabot/docker/file_parser.rb b/docker/lib/dependabot/docker/file_parser.rb index 23168f3053f..9091545fef3 100644 --- a/docker/lib/dependabot/docker/file_parser.rb +++ b/docker/lib/dependabot/docker/file_parser.rb @@ -113,6 +113,7 @@ def workfile_file_dependencies(file) # TODO: Support Docker references and path references details = string.match(IMAGE_SPEC)&.named_captures next if details.nil? + details["registry"] = nil if details["registry"] == "docker.io" version = version_from(details)