Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switch to using the new record_ecosystem_versions endpoint. #7517

Merged
merged 1 commit into from
Jul 10, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions bin/dry-run.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
2 changes: 1 addition & 1 deletion bundler/lib/dependabot/bundler/file_fetcher.rb
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ def self.required_files_message
"Repo must contain either a Gemfile, a gemspec, or a gems.rb."
end

def package_manager_version
def ecosystem_versions
{
package_managers: {
"bundler" => Helpers.detected_bundler_version(lockfile)
Expand Down
2 changes: 1 addition & 1 deletion cargo/lib/dependabot/cargo/file_fetcher.rb
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ def self.required_files_message
"Repo must contain a Cargo.toml."
end

def package_manager_version
def ecosystem_versions
channel = if rust_toolchain
TomlRB.parse(rust_toolchain.content).fetch("toolchain", nil)&.fetch("channel", nil)
else
Expand Down
6 changes: 3 additions & 3 deletions cargo/spec/dependabot/cargo/file_fetcher_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@
end

it "provides the Rust channel" do
expect(file_fetcher_instance.package_manager_version).to eq({
expect(file_fetcher_instance.ecosystem_versions).to eq({
package_managers: { "cargo" => "default" }
})
end
Expand Down Expand Up @@ -115,7 +115,7 @@
end

it "raises a DependencyFileNotParseable error" do
expect { file_fetcher_instance.package_manager_version }.to raise_error(Dependabot::DependencyFileNotParseable)
expect { file_fetcher_instance.ecosystem_versions }.to raise_error(Dependabot::DependencyFileNotParseable)
end
end

Expand Down Expand Up @@ -144,7 +144,7 @@
end

it "provides the Rust channel" do
expect(file_fetcher_instance.package_manager_version).to eq({
expect(file_fetcher_instance.ecosystem_versions).to eq({
package_managers: { "cargo" => "1.2.3" }
})
end
Expand Down
2 changes: 1 addition & 1 deletion common/lib/dependabot/file_fetchers/base.rb
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ def clone_repo_contents
raise Dependabot::RepoNotFound, source
end

def package_manager_version
def ecosystem_versions
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs a code comment explaining the expected shape of the data, or possibly a doc somewhere for devs, but we're still noodling internally on what we want this to look like, so punting on those docs until later.

Right now just want to get it swapped over to this new endpoint so can start prototyping actual code to see how my design ideas hold up to reality--ie, do we want to record more/different info then I expect.

nil
end

Expand Down
2 changes: 1 addition & 1 deletion go_modules/lib/dependabot/go_modules/file_fetcher.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ def self.required_files_message
"Repo must contain a go.mod."
end

def package_manager_version
def ecosystem_versions
return nil unless go_mod

{
Expand Down
4 changes: 2 additions & 2 deletions go_modules/spec/dependabot/go_modules/file_fetcher_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@
end

it "provides the Go modules version" do
expect(file_fetcher_instance.package_manager_version).to eq({
expect(file_fetcher_instance.ecosystem_versions).to eq({
package_managers: { "gomod" => "unknown" }
})
end
Expand Down Expand Up @@ -77,7 +77,7 @@
end

it "provides the Go modules version" do
expect(file_fetcher_instance.package_manager_version).to eq({
expect(file_fetcher_instance.ecosystem_versions).to eq({
package_managers: { "gomod" => "1.19" }
})
end
Expand Down
2 changes: 1 addition & 1 deletion npm_and_yarn/lib/dependabot/npm_and_yarn/file_fetcher.rb
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ def clone_repo_contents
end
end

def package_manager_version
def ecosystem_versions
package_managers = {}

package_managers["npm"] = Helpers.npm_version_numeric(package_lock.content) if package_lock
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -282,7 +282,7 @@
end

it "parses the yarn lockfile" do
expect(file_fetcher_instance.package_manager_version).to eq(
expect(file_fetcher_instance.ecosystem_versions).to eq(
{ package_managers: { "yarn" => 1 } }
)
end
Expand Down Expand Up @@ -340,7 +340,7 @@
end

it "parses the shrinkwrap file" do
expect(file_fetcher_instance.package_manager_version).to eq(
expect(file_fetcher_instance.ecosystem_versions).to eq(
{ package_managers: { "shrinkwrap" => 1 } }
)
end
Expand Down Expand Up @@ -373,7 +373,7 @@
end

it "parses the npm lockfile" do
expect(file_fetcher_instance.package_manager_version).to eq(
expect(file_fetcher_instance.ecosystem_versions).to eq(
{ package_managers: { "npm" => 6 } }
)
end
Expand Down Expand Up @@ -410,7 +410,7 @@
end

it "parses the package manager version" do
expect(file_fetcher_instance.package_manager_version).to eq(
expect(file_fetcher_instance.ecosystem_versions).to eq(
{ package_managers: { "npm" => 6, "yarn" => 1 } }
)
end
Expand Down
9 changes: 3 additions & 6 deletions updater/lib/dependabot/api_client.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
jeffwidman marked this conversation as resolved.
Show resolved Hide resolved
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
Expand Down
7 changes: 2 additions & 5 deletions updater/lib/dependabot/file_fetcher_command.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion updater/lib/dependabot/service.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
6 changes: 3 additions & 3 deletions updater/spec/dependabot/api_client_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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).
Expand Down
5 changes: 1 addition & 4 deletions updater/spec/dependabot/file_fetcher_command_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -109,9 +109,6 @@
allow_any_instance_of(Dependabot::Bundler::FileFetcher).
to receive(:files).
and_raise(exception)
allow_any_instance_of(Dependabot::Bundler::FileFetcher).
to receive(:package_manager_version).
and_return(nil)
end

it "retries the job when the rate-limit is reset and reports api error" do
Expand Down
7 changes: 2 additions & 5 deletions updater/spec/dependabot/integration_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -51,14 +51,13 @@
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)
commit: "sha")
end
let(:message_builder) do
instance_double(Dependabot::PullRequestCreator::MessageBuilder, message: nil)
Expand All @@ -79,8 +78,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)
jeffwidman marked this conversation as resolved.
Show resolved Hide resolved
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)

Expand Down
2 changes: 1 addition & 1 deletion updater/spec/dependabot/service_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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) }

Expand Down