From 1c173d83b06288663119ed4d8deacd8203c90959 Mon Sep 17 00:00:00 2001 From: Jeff Widman Date: Mon, 3 Jul 2023 21:58:36 -0700 Subject: [PATCH] Switch to the `record_ecosystem_versions` endpoint This flips the `updater` to calling the new `record_ecosystem_versions` endpoint. Note that this does _not_ flip the per-ecosystem instances of those metrics. That will be done in separate PR's for easier review/discussion/audit of those individual ecosystems. Co-authored-by: Barry Gordon <896971+brrygrdn@users.noreply.github.com> --- bin/dry-run.rb | 4 ++-- common/lib/dependabot/file_fetchers/base.rb | 2 +- updater/lib/dependabot/api_client.rb | 9 +++------ updater/lib/dependabot/file_fetcher_command.rb | 7 ++----- updater/lib/dependabot/service.rb | 2 +- updater/spec/dependabot/api_client_spec.rb | 6 +++--- updater/spec/dependabot/file_fetcher_command_spec.rb | 5 +++-- updater/spec/dependabot/integration_spec.rb | 7 +++---- updater/spec/dependabot/service_spec.rb | 2 +- 9 files changed, 19 insertions(+), 25 deletions(-) diff --git a/bin/dry-run.rb b/bin/dry-run.rb index 7b9266083cc9..7020df2dbce1 100755 --- a/bin/dry-run.rb +++ b/bin/dry-run.rb @@ -799,8 +799,8 @@ def security_fix?(dependency) StackProf.results("tmp/stackprof-#{Time.now.strftime('%Y-%m-%d-%H:%M')}.dump") if $options[:profile] puts "🌍 Total requests made: '#{$network_trace_count}'" -package_manager = fetcher.package_manager_version -puts "🎈 Package manager version log: #{package_manager}" unless package_manager.nil? +ecosystem_versions = fetcher.ecosystem_versions +puts "🎈 Ecosystem Versions log: #{ecosystem_versions}" unless ecosystem_versions.nil? # rubocop:enable Metrics/BlockLength diff --git a/common/lib/dependabot/file_fetchers/base.rb b/common/lib/dependabot/file_fetchers/base.rb index 6c835e874024..60bb61e84ac7 100644 --- a/common/lib/dependabot/file_fetchers/base.rb +++ b/common/lib/dependabot/file_fetchers/base.rb @@ -102,7 +102,7 @@ def clone_repo_contents raise Dependabot::RepoNotFound, source end - def package_manager_version + def ecosystem_versions nil end diff --git a/updater/lib/dependabot/api_client.rb b/updater/lib/dependabot/api_client.rb index 64644422de48..440b04393c74 100644 --- a/updater/lib/dependabot/api_client.rb +++ b/updater/lib/dependabot/api_client.rb @@ -119,13 +119,10 @@ def update_dependency_list(dependencies, dependency_files) sleep(rand(3.0..10.0)) && retry end - def record_package_manager_version(package_managers) - api_url = "#{base_url}/update_jobs/#{job_id}/record_package_manager_version" + def record_ecosystem_versions(ecosystem_versions) + api_url = "#{base_url}/update_jobs/#{job_id}/record_ecosystem_versions" body = { - data: { - ecosystem: "deprecated", - "package-managers": package_managers - } + data: { "ecosystem_versions": ecosystem_versions } } response = http_client.post(api_url, json: body) raise ApiError, response.body if response.code >= 400 diff --git a/updater/lib/dependabot/file_fetcher_command.rb b/updater/lib/dependabot/file_fetcher_command.rb index 768547551139..6678e4512abb 100644 --- a/updater/lib/dependabot/file_fetcher_command.rb +++ b/updater/lib/dependabot/file_fetcher_command.rb @@ -22,12 +22,9 @@ def perform_job raise "base commit SHA not found" unless @base_commit_sha # We don't set this flag in GHES because there's no point in recording versions since we can't access that data. - # TODO: The flag is named `record_ecosystem_versions` because `package_manager_version` is getting renamed to - # to that shortly... but splitting into separate PR's for lower risk/easiery testability. Once the follow-on PR - # with the rename lands, the name confusion will disappear. if Experiments.enabled?(:record_ecosystem_versions) - version = file_fetcher.package_manager_version - api_client.record_package_manager_version(version[:package_managers]) unless version.nil? + ecosystem_versions = file_fetcher.ecosystem_versions + api_client.record_ecosystem_versions(ecosystem_versions) unless ecosystem_versions.nil? end dependency_files diff --git a/updater/lib/dependabot/service.rb b/updater/lib/dependabot/service.rb index 8a4e5eedb116..51a99f92238d 100644 --- a/updater/lib/dependabot/service.rb +++ b/updater/lib/dependabot/service.rb @@ -24,7 +24,7 @@ def initialize(client:) def_delegators :client, :mark_job_as_processed, :update_dependency_list, - :record_package_manager_version, + :record_ecosystem_versions, :increment_metric def create_pull_request(dependency_change, base_commit_sha) diff --git a/updater/spec/dependabot/api_client_spec.rb b/updater/spec/dependabot/api_client_spec.rb index bc91d4d5a2ac..630685a23899 100644 --- a/updater/spec/dependabot/api_client_spec.rb +++ b/updater/spec/dependabot/api_client_spec.rb @@ -384,12 +384,12 @@ end end - describe "record_package_manager_version" do - let(:url) { "http://example.com/update_jobs/1/record_package_manager_version" } + describe "ecosystem_versions" do + let(:url) { "http://example.com/update_jobs/1/record_ecosystem_versions" } before { stub_request(:post, url).to_return(status: 204) } it "hits the correct endpoint" do - client.record_package_manager_version({ "bundler" => "2" }) + client.record_ecosystem_versions({ "ruby" => { "min" => 3, "max" => 3.2 }}) expect(WebMock). to have_requested(:post, url). diff --git a/updater/spec/dependabot/file_fetcher_command_spec.rb b/updater/spec/dependabot/file_fetcher_command_spec.rb index 892039ce323c..7135c5749ca7 100644 --- a/updater/spec/dependabot/file_fetcher_command_spec.rb +++ b/updater/spec/dependabot/file_fetcher_command_spec.rb @@ -17,7 +17,7 @@ allow(api_client).to receive(:mark_job_as_processed) allow(api_client).to receive(:record_update_job_error) - allow(api_client).to receive(:record_package_manager_version) + allow(api_client).to receive(:record_ecosystem_versions) allow(Dependabot::Environment).to receive(:output_path).and_return(File.join(Dir.mktmpdir, "output.json")) allow(Dependabot::Environment).to receive(:job_id).and_return(job_id) @@ -109,8 +109,9 @@ allow_any_instance_of(Dependabot::Bundler::FileFetcher). to receive(:files). and_raise(exception) + # TODO this may be deletable since the ecosystem_versions call is now buried behind a flag... allow_any_instance_of(Dependabot::Bundler::FileFetcher). - to receive(:package_manager_version). + to receive(:ecosystem_versions). and_return(nil) end diff --git a/updater/spec/dependabot/integration_spec.rb b/updater/spec/dependabot/integration_spec.rb index 323cf2397003..b05dcc08fce3 100644 --- a/updater/spec/dependabot/integration_spec.rb +++ b/updater/spec/dependabot/integration_spec.rb @@ -51,14 +51,15 @@ mark_job_as_processed: nil, update_dependency_list: nil, record_update_job_error: nil, - record_package_manager_version: nil, + record_ecosystem_versions: nil, increment_metric: nil) end let(:file_fetcher) do instance_double(Dependabot::FileFetchers::Base, files: dependency_files, commit: "sha", - package_manager_version: nil) + # TODO this may be deletable since the ecosystem_versions call is now buried behind a flag... + ecosystem_versions: nil) end let(:message_builder) do instance_double(Dependabot::PullRequestCreator::MessageBuilder, message: nil) @@ -79,8 +80,6 @@ # Stub Dependabot object with instance doubles allow(Dependabot::ApiClient).to receive(:new).and_return(api_client) - # Recording the package manager happens via an observer so the instantiated `api_client` does not receive this call - allow_any_instance_of(Dependabot::ApiClient).to receive(:record_package_manager_version) allow(Dependabot::FileFetchers).to receive_message_chain(:for_package_manager, :new).and_return(file_fetcher) allow(Dependabot::PullRequestCreator::MessageBuilder).to receive(:new).and_return(message_builder) diff --git a/updater/spec/dependabot/service_spec.rb b/updater/spec/dependabot/service_spec.rb index eb713ad9ea1e..44eb583a897a 100644 --- a/updater/spec/dependabot/service_spec.rb +++ b/updater/spec/dependabot/service_spec.rb @@ -160,7 +160,7 @@ describe "Instance methods delegated to @client" do { mark_job_as_processed: %w(mock_sha), - record_package_manager_version: %w(mock_package_managers) + record_ecosystem_versions: %w(mock_ecosystem_versions) }.each do |method, arguments| before { allow(mock_client).to receive(method) }