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

Remove Bundler v1 Deprecation and Unsupported Feature Flags and Deactivate Bundler v1 CI Tests #10796

Merged
merged 13 commits into from
Oct 16, 2024
Merged
3 changes: 1 addition & 2 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,7 @@ jobs:
fail-fast: false
matrix:
suite:
- { path: bundler, name: bundler1, ecosystem: bundler }
- { path: bundler, name: bundler2, ecosystem: bundler }
- { path: bundler, name: bundler, ecosystem: bundler }
- { path: cargo, name: cargo, ecosystem: cargo }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Remove bundler1 from test suites

- { path: common, name: common, ecosystem: common}
- { path: composer, name: composer, ecosystem: composer }
Expand Down
20 changes: 9 additions & 11 deletions bin/ci-test
Original file line number Diff line number Diff line change
Expand Up @@ -10,17 +10,15 @@ function print_usage() {

function handle_args() {
export SUITE_NAME=$1
case $SUITE_NAME in
bundler1 | bundler2)
export MODULE=bundler
;;
"")
print_usage
exit 1
;;
*)
export MODULE=$SUITE_NAME
esac

# Only allow "bundler" as a valid SUITE_NAME
if [[ "$SUITE_NAME" != "bundler" ]]; then
echo "Error: Only 'bundler' is allowed as SUITE_NAME."
print_usage
exit 1
fi

export MODULE="bundler"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since we are removing bundler v1 from specs and replace suite name as bundler which is going to run specs for bundler v2 directly.


if ! [ -d "$MODULE" ]; then
print_usage
Expand Down
3 changes: 1 addition & 2 deletions bundler/Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,7 @@ FROM ghcr.io/dependabot/dependabot-updater-core
USER dependabot

COPY --chown=dependabot:dependabot bundler/helpers /opt/bundler/helpers
RUN bash /opt/bundler/helpers/v1/build \
&& bash /opt/bundler/helpers/v2/build
RUN bash /opt/bundler/helpers/v2/build
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't need to build old bundler v1 code anymore


COPY --chown=dependabot:dependabot bundler $DEPENDABOT_HOME/bundler
COPY --chown=dependabot:dependabot common $DEPENDABOT_HOME/common
Expand Down
1 change: 1 addition & 0 deletions bundler/lib/dependabot/bundler/file_updater.rb
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,7 @@ def updated_gemspec_content(gemspec)
end

def updated_lockfile_content
puts "It is here"
@updated_lockfile_content ||=
LockfileUpdater.new(
dependencies: dependencies,
Expand Down
12 changes: 4 additions & 8 deletions bundler/lib/dependabot/bundler/helpers.rb
Original file line number Diff line number Diff line change
Expand Up @@ -20,21 +20,17 @@ def self.bundler_version(lockfile)

if (matches = lockfile.content&.match(BUNDLER_MAJOR_VERSION_REGEX))
matches[:version].to_i >= 2 ? V2 : V1
elsif Dependabot::Experiments.enabled?(:bundler_v1_unsupported_error)
DEFAULT
else
failover_version
DEFAULT
end
end

# If we are updating a project with a Gemfile.lock that does not specify
# the version it was bundled with, we failover to V1 on the assumption
# it was created with an old version that didn't add this information
# the version it was bundled with, we failover to V2
# the version it was bundled with, we failover to V2
sig { returns(String) }
def self.failover_version
return V2 if Dependabot::Experiments.enabled?(:bundler_v1_unsupported_error)

V1
V2
end
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are going to always return bundler v2 except if version defined lower than version 2. When version is lower than version 2 we assume it is going to be V1 and our unsupported error is going to throw error for that.

Copy link
Member

Choose a reason for hiding this comment

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

Is failover_version being used anywhere now? Maybe it can be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is removed.


sig { params(lockfile: T.nilable(Dependabot::DependencyFile)).returns(String) }
Expand Down
12 changes: 6 additions & 6 deletions bundler/lib/dependabot/bundler/package_manager.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,11 @@ module Bundler
# Keep versions in ascending order
SUPPORTED_BUNDLER_VERSIONS = T.let([Version.new("2")].freeze, T::Array[Dependabot::Version])

DEPRECATED_BUNDLER_VERSIONS = T.let([
Version.new("1")
].freeze, T::Array[Dependabot::Version])
# Currently, we don't support any deprecated versions of Bundler
# When a version is going to be unsupported, it will be added here for a while to give users time to upgrade
# Example for deprecation:
# DEPRECATED_BUNDLER_VERSIONS = T.let([Version.new("1")].freeze, T::Array[Dependabot::Version])
DEPRECATED_BUNDLER_VERSIONS = T.let([].freeze, T::Array[Dependabot::Version])

class PackageManager < PackageManagerBase
extend T::Sig
Expand All @@ -41,9 +43,7 @@ def initialize(version)

sig { override.returns(T::Boolean) }
def unsupported?
# Check if the feature flag for Bundler v1 unsupported error is enabled.
return false unless Dependabot::Experiments.enabled?(:bundler_v1_unsupported_error)

# Check if the version is not supported
supported_versions.all? { |supported| supported > version }
end
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,7 @@ module SharedBundlerHelpers
PATH_REGEX = /The path `(?<path>.*)` does not exist/

module BundlerErrorPatterns
# The `set --global` optional part can be made required when Bundler 1 support is dropped
MISSING_AUTH_REGEX = /bundle config (?:set --global )?(?<source>.*) username:password/
MISSING_AUTH_REGEX = /bundle config set --global (?<source>.*) username:password/

BAD_AUTH_REGEX = /Bad username or password for (?<source>.*)\.$/
FORBIDDEN_AUTH_REGEX = /Access token could not be authenticated for (?<source>.*)\.$/
Expand Down
15 changes: 4 additions & 11 deletions bundler/script/ci-test
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,7 @@ bundle exec turbo_tests --verbose
# NOTE: Don't use `if` branches without `else` part, since the code in some of
# then seems to not abort the script regardless of `set -e`

if [[ "$SUITE_NAME" == "bundler1" ]]; then
# shellcheck source=../helpers/v1/build
DEPENDABOT_NATIVE_HELPERS_PATH="" source helpers/v1/build \
&& BUNDLER_VERSION=1.17.3 ./patched_bundler exec rspec spec\
&& cd -
else
# shellcheck source=../helpers/v2/build
DEPENDABOT_NATIVE_HELPERS_PATH="" source helpers/v2/build \
&& bundle exec rspec spec \
&& cd -
fi
# shellcheck source=../helpers/v2/build
DEPENDABOT_NATIVE_HELPERS_PATH="" source helpers/v2/build \
&& bundle exec rspec spec \
&& cd -
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We only need to run specs for bundler v2 version.

Original file line number Diff line number Diff line change
Expand Up @@ -19,15 +19,7 @@
its(:content) { is_expected.to include('gem "business", "~> 1.4.0"') }
end

describe "the updated lockfile", :bundler_v1_only do
subject do
prepared_dependency_files.find { |f| f.name == "Gemfile.lock" }
end

its(:content) { is_expected.to include("1.10.6") }
end

describe "the updated lockfile", :bundler_v2_only do
describe "the updated lockfile" do
subject do
prepared_dependency_files.find { |f| f.name == "Gemfile.lock" }
end
Expand Down
124 changes: 3 additions & 121 deletions bundler/spec/dependabot/bundler/file_parser_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -172,32 +172,7 @@

its(:length) { is_expected.to eq(5) }

describe "an untagged dependency", :bundler_v1_only do
subject { dependencies.find { |d| d.name == "uk_phone_numbers" } }

let(:expected_requirements) do
[{
requirement: ">= 0",
file: "Gemfile",
source: {
type: "git",
url: "https://github.com/dependabot-fixtures/uk_phone_numbers",
branch: nil,
ref: "master"
},
groups: [:default]
}]
end

it { is_expected.to be_a(Dependabot::Dependency) }
its(:requirements) { is_expected.to eq(expected_requirements) }

its(:version) do
is_expected.to eq("1530024bd6a68d36ac18e04836ce110e0d433c36")
end
end

describe "an untagged dependency", :bundler_v2_only do
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't need bundler v1 only specs anymore

describe "an untagged dependency" do
subject { dependencies.find { |d| d.name == "uk_phone_numbers" } }

let(:expected_requirements) do
Expand Down Expand Up @@ -247,34 +222,7 @@
end
end

describe "a github dependency", :bundler_v1_only do
subject { dependencies.find { |d| d.name == "business" } }

let(:dependency_files) { bundler_project_dependency_files("github_source") }

let(:expected_requirements) do
[{
requirement: ">= 0",
file: "Gemfile",
source: {
type: "git",
url: "https://github.com/dependabot-fixtures/business.git",
branch: nil,
ref: "master"
},
groups: [:default]
}]
end

it { is_expected.to be_a(Dependabot::Dependency) }
its(:requirements) { is_expected.to eq(expected_requirements) }

its(:version) do
is_expected.to eq("d31e445215b5af70c1604715d97dd953e868380e")
end
end

describe "a github dependency", :bundler_v2_only do
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't need bundler v1 only specs anymore

describe "a github dependency" do
subject { dependencies.find { |d| d.name == "business" } }

let(:dependency_files) { bundler_project_dependency_files("github_source") }
Expand All @@ -301,31 +249,7 @@
end
end

context "with a subdependency of a git source", :bundler_v1_only do
subject { dependencies.find { |d| d.name == "kaminari-actionview" } }

let(:dependency_files) { bundler_project_dependency_files("git_source_undeclared") }

let(:expected_requirements) do
[{
requirement: ">= 0",
file: "Gemfile",
source: {
type: "git",
url: "https://github.com/dependabot-fixtures/kaminari",
branch: nil,
ref: "master"
},
groups: [:default]
}]
end

it { is_expected.to be_a(Dependabot::Dependency) }
its(:name) { is_expected.to eq("kaminari-actionview") }
its(:requirements) { is_expected.to eq(expected_requirements) }
end

context "with a subdependency of a git source", :bundler_v2_only do
context "with a subdependency of a git source" do
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't need bundler v1 only specs anymore

subject { dependencies.find { |d| d.name == "kaminari-actionview" } }

let(:dependency_files) { bundler_project_dependency_files("git_source_undeclared") }
Expand Down Expand Up @@ -589,48 +513,6 @@
end
end

context "with an unparseable git dep that also appears in the gemspec", :bundler_v1_only do
let(:dependency_files) { bundler_project_dependency_files("git_source_unparseable") }

it "includes source details on the gemspec requirement" do
expect(dependencies.map(&:name)).to match_array(%w(business))
expect(dependencies.first.name).to eq("business")
expect(dependencies.first.version)
.to eq("1378a2b0b446d991b7567efbc7eeeed2720e4d8f")
expect(dependencies.first.requirements)
.to contain_exactly({
file: "example.gemspec",
requirement: "~> 1.0",
groups: ["runtime"],
source: {
type: "git",
url: "git@github.com:dependabot-fixtures/business",
branch: nil,
ref: "master"
}
})
end

it "includes source details on the gemspec requirement", :bundler_v2_only do
expect(dependencies.map(&:name)).to match_array(%w(business))
expect(dependencies.first.name).to eq("business")
expect(dependencies.first.version)
.to eq("1378a2b0b446d991b7567efbc7eeeed2720e4d8f")
expect(dependencies.first.requirements)
.to contain_exactly({
file: "example.gemspec",
requirement: "~> 1.0",
groups: ["runtime"],
source: {
type: "git",
url: "git@github.com:dependabot-fixtures/business",
branch: nil,
ref: nil
}
})
end
end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't need bundler v1 only specs anymore

context "with two gemspecs" do
let(:dependency_files) { bundler_project_dependency_files("imports_two_gemspecs") }

Expand Down
Loading
Loading