Skip to content

Introduce group rule to branch namer #6784

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.

Sign up for GitHub

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

Merged
landongrindheim merged 7 commits into from
Mar 6, 2023
Merged

Introduce group rule to branch namer #6784

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
de007dc
Extract current naming logic into a solo strategy
landongrindheim Mar 3, 2023
553b3e9
Move solo strategy into its own file
landongrindheim Mar 3, 2023
6fcca58
Duplicate specs into new file
landongrindheim Mar 3, 2023
161bb61
Introduce GroupRuleStrategy
landongrindheim Mar 3, 2023
a79c172
Delegate to strategy based on group rule presence
landongrindheim Mar 3, 2023
6010c76
Introduce the GroupRule concept
landongrindheim Mar 3, 2023
56911b0
Pass `group_rule` to BranchNamer
landongrindheim Mar 3, 2023
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
11 changes: 11 additions & 0 deletions common/lib/dependabot/group_rule.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
# frozen_string_literal: true

module Dependabot
class GroupRule
Copy link
Copy Markdown
Contributor Author

@landongrindheim landongrindheim Mar 6, 2023

Choose a reason for hiding this comment

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

I'm a little unclear on the interfact for a GroupRule. The rule portion signifies a policy-like approach, where criteria must be met. I'm open to this changing, or to refining the concept as we move forward.

attr_reader :name

def initialize(name)
@name = name
end
end
end
1 change: 1 addition & 0 deletions common/lib/dependabot/pull_request_creator.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,7 @@ def branch_namer
dependencies: dependencies,
files: files,
target_branch: source.branch,
group_rule: nil,
separator: branch_name_separator,
prefix: branch_name_prefix,
max_length: branch_name_max_length
Expand Down
207 changes: 28 additions & 179 deletions common/lib/dependabot/pull_request_creator/branch_namer.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,203 +4,52 @@

require "dependabot/metadata_finders"
require "dependabot/pull_request_creator"
require "dependabot/pull_request_creator/branch_namer/solo_strategy"

module Dependabot
class PullRequestCreator
class BranchNamer
attr_reader :dependencies, :files, :target_branch, :separator, :prefix, :max_length
attr_reader :dependencies, :files, :target_branch, :separator, :prefix, :max_length, :group_rule

def initialize(dependencies:, files:, target_branch:, separator: "/",
prefix: "dependabot", max_length: nil)
def initialize(dependencies:, files:, target_branch:, group_rule: nil,
separator: "/", prefix: "dependabot", max_length: nil)
@dependencies = dependencies
@files = files
@target_branch = target_branch
@group_rule = group_rule
@separator = separator
@prefix = prefix
@max_length = max_length
end

def new_branch_name
@name ||=
begin
dependency_name_part =
if dependencies.count > 1 && updating_a_property?
property_name
elsif dependencies.count > 1 && updating_a_dependency_set?
dependency_set.fetch(:group)
else
dependencies.
map(&:name).
join("-and-").
tr(":[]", "-").
tr("@", "")
end

"#{dependency_name_part}-#{branch_version_suffix}"
end

# Some users need branch names without slashes
sanitized_name = sanitize_ref(File.join(prefixes, @name).gsub("/", separator))

# Shorten the ref in case users refs have length limits
if @max_length && (sanitized_name.length > @max_length)
sha = Digest::SHA1.hexdigest(sanitized_name)[0, @max_length]
sanitized_name[[@max_length - sha.size, 0].max..] = sha
end

sanitized_name
strategy.new_branch_name
end

private

def prefixes
[
prefix,
package_manager,
files.first.directory.tr(" ", "-"),
target_branch
].compact
end

def package_manager
dependencies.first.package_manager
end

def updating_a_property?
dependencies.first.
requirements.
any? { |r| r.dig(:metadata, :property_name) }
end

def updating_a_dependency_set?
dependencies.first.
requirements.
any? { |r| r.dig(:metadata, :dependency_set) }
end

def property_name
@property_name ||= dependencies.first.requirements.
find { |r| r.dig(:metadata, :property_name) }&.
dig(:metadata, :property_name)

raise "No property name!" unless @property_name

@property_name
end

def dependency_set
@dependency_set ||= dependencies.first.requirements.
find { |r| r.dig(:metadata, :dependency_set) }&.
dig(:metadata, :dependency_set)

raise "No dependency set!" unless @dependency_set

@dependency_set
end

def branch_version_suffix
dep = dependencies.first

if dep.removed?
"-removed"
elsif library? && ref_changed?(dep) && new_ref(dep)
new_ref(dep)
elsif library?
sanitized_requirement(dep)
else
new_version(dep)
end
end

def sanitized_requirement(dependency)
new_library_requirement(dependency).
delete(" ").
gsub("!=", "neq-").
gsub(">=", "gte-").
gsub("<=", "lte-").
gsub("~>", "tw-").
gsub("^", "tw-").
gsub("||", "or-").
gsub("~", "approx-").
gsub("~=", "tw-").
gsub(/==*/, "eq-").
gsub(">", "gt-").
gsub("<", "lt-").
gsub("*", "star").
gsub(",", "-and-")
end

def new_version(dependency)
# Version looks like a git SHA and we could be updating to a specific
# ref in which case we return that otherwise we return a shorthand sha
if dependency.version.match?(/^[0-9a-f]{40}$/)
return new_ref(dependency) if ref_changed?(dependency) && new_ref(dependency)

dependency.version[0..6]
elsif dependency.version == dependency.previous_version &&
package_manager == "docker"
dependency.requirements.
filter_map { |r| r.dig(:source, "digest") || r.dig(:source, :digest) }.
first.split(":").last[0..6]
else
dependency.version
end
end

def previous_ref(dependency)
previous_refs = dependency.previous_requirements.filter_map do |r|
r.dig(:source, "ref") || r.dig(:source, :ref)
end.uniq
return previous_refs.first if previous_refs.count == 1
end

def new_ref(dependency)
new_refs = dependency.requirements.filter_map do |r|
r.dig(:source, "ref") || r.dig(:source, :ref)
end.uniq
return new_refs.first if new_refs.count == 1
end

def ref_changed?(dependency)
# We could go from multiple previous refs (nil) to a single new ref
previous_ref(dependency) != new_ref(dependency)
end

def new_library_requirement(dependency)
updated_reqs =
dependency.requirements - dependency.previous_requirements

gemspec =
updated_reqs.find { |r| r[:file].match?(%r{^[^/]*\.gemspec$}) }
return gemspec[:requirement] if gemspec

updated_reqs.first[:requirement]
end

# TODO: Bring this in line with existing library checks that we do in the
# update checkers, which are also overriden by passing an explicit
# `requirements_update_strategy`.
#
# TODO re-use in MessageBuilder
def library?
dependencies.any? { |d| !d.appears_in_lockfile? }
end

def requirements_changed?(dependency)
(dependency.requirements - dependency.previous_requirements).any?
end

def sanitize_ref(ref)
# This isn't a complete implementation of git's ref validation, but it
# covers most cases that crop up. Its list of allowed characters is a
# bit stricter than git's, but that's for cosmetic reasons.
ref.
# Remove forbidden characters (those not already replaced elsewhere)
gsub(%r{[^A-Za-z0-9/\-_.(){}]}, "").
# Slashes can't be followed by periods
gsub(%r{/\.}, "/dot-").squeeze(".").squeeze("/").
# Trailing periods are forbidden
sub(/\.$/, "")
def strategy
@strategy ||=
if group_rule.nil?
SoloStrategy.new(
dependencies: dependencies,
files: files,
target_branch: target_branch,
separator: separator,
prefix: prefix,
max_length: max_length
)
else
GroupRuleStrategy.new(
dependencies: dependencies,
files: files,
target_branch: target_branch,
group_rule: group_rule,
separator: separator,
prefix: prefix,
max_length: max_length
)
end
end
end
end
Expand Down
28 changes: 28 additions & 0 deletions common/lib/dependabot/pull_request_creator/branch_namer/group_rule_strategy.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
# frozen_string_literal: true

module Dependabot
class PullRequestCreator
class BranchNamer
class GroupRuleStrategy
def initialize(dependencies:, files:, target_branch:, group_rule:,
separator: "/", prefix: "dependabot", max_length: nil)
@dependencies = dependencies
@files = files
@target_branch = target_branch
@group_rule = group_rule
@separator = separator
@prefix = prefix
@max_length = max_length
end

def new_branch_name
group_rule.name
Copy link
Copy Markdown
Contributor

@brrygrdn brrygrdn Mar 6, 2023

Choose a reason for hiding this comment

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

We should probably expect a group rule name to be a humanized string, as we'll need to either:

  • Validate it strictly as being a valid branch name in the config file
  • Accept any string and 'slugify' it here replacing invalid characters/spaces etc

I'd tend to prefer the latter as while it might end up making sense to validate it in the config for other reasons, this component probably shouldn't trust its inputs?

Copy link
Copy Markdown
Contributor Author

@landongrindheim landongrindheim Mar 6, 2023

Choose a reason for hiding this comment

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

  • Accept any string and 'slugify' it here replacing invalid characters/spaces etc

I'd tend to prefer the latter as while it might end up making sense to validate it in the config for other reasons, this component probably shouldn't trust its inputs?

That makes sense to me. I'm going to leave this as is for now, in its very naive state, so we can make decisions on this when we have more information.

end

private

attr_reader :group_rule
end
end
end
end
Loading