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

utils/github: use GraphQL PR searching #16886

Merged
merged 1 commit into from
Mar 16, 2024
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 1 addition & 5 deletions Library/Homebrew/dev-cmd/bump.rb
Original file line number Diff line number Diff line change
Expand Up @@ -312,11 +312,7 @@ def retrieve_pull_requests(formula_or_cask, name, state:, version: nil)
nil
end

if pull_requests&.any?
pull_requests = pull_requests.map { |pr| "#{pr["title"]} (#{Formatter.url(pr["html_url"])})" }.join(", ")
end

pull_requests
pull_requests&.map { |pr| "#{pr["title"]} (#{Formatter.url(pr["html_url"])})" }&.join(", ")
end

sig {
Expand Down
98 changes: 81 additions & 17 deletions Library/Homebrew/utils/github.rb
Original file line number Diff line number Diff line change
Expand Up @@ -516,19 +516,67 @@
/(^|\s)#{Regexp.quote(name)}(:|,|\s)(.*\s)?#{Regexp.quote(version)}(:|,|\s|$)/i
end

sig {
params(name: String, tap_remote_repo: String, state: T.nilable(String), version: T.nilable(String))

Check warning on line 520 in Library/Homebrew/utils/github.rb

View check run for this annotation

Codecov / codecov/patch

Library/Homebrew/utils/github.rb#L520

Added line #L520 was not covered by tests
.returns(T::Array[T::Hash[String, T.untyped]])
}
def self.fetch_pull_requests(name, tap_remote_repo, state: nil, version: nil)
regex = pull_request_title_regex(name, version)
query = "is:pr #{name} #{version}".strip

issues_for_formula(query, tap_remote_repo:, state:).select do |pr|
pr["html_url"].include?("/pull/") && regex.match?(pr["title"])
# Unauthenticated users cannot use GraphQL so use search REST API instead.
# Limit for this is 30/minute so is usually OK unless you're spamming bump PRs (e.g. CI).
if API.credentials_type == :none

Check warning on line 529 in Library/Homebrew/utils/github.rb

View check run for this annotation

Codecov / codecov/patch

Library/Homebrew/utils/github.rb#L529

Added line #L529 was not covered by tests
return issues_for_formula(query, tap_remote_repo:, state:).select do |pr|
pr["html_url"].include?("/pull/") && regex.match?(pr["title"])

Check warning on line 531 in Library/Homebrew/utils/github.rb

View check run for this annotation

Codecov / codecov/patch

Library/Homebrew/utils/github.rb#L531

Added line #L531 was not covered by tests
end
elsif state == "open" && ENV["GITHUB_REPOSITORY_OWNER"] == "Homebrew"
# Try use PR API, which might be cheaper on rate limits in some cases.
# The rate limit of the search API under GraphQL is unclear as it's
# costs the same as any other query accoding to /rate_limit.
# The PR API is also not very scalable so limit to Homebrew CI.
return fetch_open_pull_requests(name, tap_remote_repo, version:)
end

query += " repo:#{tap_remote_repo} in:title"

Check warning on line 541 in Library/Homebrew/utils/github.rb

View check run for this annotation

Codecov / codecov/patch

Library/Homebrew/utils/github.rb#L541

Added line #L541 was not covered by tests
query += " state:#{state}" if state.present?
graphql_query = <<~EOS

Check warning on line 543 in Library/Homebrew/utils/github.rb

View check run for this annotation

Codecov / codecov/patch

Library/Homebrew/utils/github.rb#L543

Added line #L543 was not covered by tests
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: This probably makes more sense as a constant.

Copy link
Member Author

@Bo98 Bo98 Mar 15, 2024

Choose a reason for hiding this comment

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

frozen_string_literal already makes this a single memory object across function calls and it is very specific to this method itself so making it global feels weird.

query($query: String!, $after: String) {
search(query: $query, type: ISSUE, first: 100, after: $after) {
nodes {
... on PullRequest {
number
title
url
state
}
}
pageInfo {
hasNextPage
endCursor
}
}
}
EOS
variables = { query: }

Check warning on line 561 in Library/Homebrew/utils/github.rb

View check run for this annotation

Codecov / codecov/patch

Library/Homebrew/utils/github.rb#L561

Added line #L561 was not covered by tests

pull_requests = []
API.paginate_graphql(graphql_query, variables:) do |result|
data = result["search"]
pull_requests.concat(data["nodes"].select { |pr| regex.match?(pr["title"]) })
data["pageInfo"]

Check warning on line 567 in Library/Homebrew/utils/github.rb

View check run for this annotation

Codecov / codecov/patch

Library/Homebrew/utils/github.rb#L563-L567

Added lines #L563 - L567 were not covered by tests
end
pull_requests.map! do |pr|
pr.merge({

Check warning on line 570 in Library/Homebrew/utils/github.rb

View check run for this annotation

Codecov / codecov/patch

Library/Homebrew/utils/github.rb#L569-L570

Added lines #L569 - L570 were not covered by tests
"html_url" => pr.delete("url"),
"state" => pr.fetch("state").downcase,
})
end
rescue API::RateLimitExceededError => e
opoo e.message
[]
pull_requests || []

Check warning on line 577 in Library/Homebrew/utils/github.rb

View check run for this annotation

Codecov / codecov/patch

Library/Homebrew/utils/github.rb#L577

Added line #L577 was not covered by tests
end

# WARNING: The GitHub API returns results in a slightly different form here compared to `fetch_pull_requests`.
def self.fetch_open_pull_requests(name, tap_remote_repo, version: nil)
return [] if tap_remote_repo.blank?

Expand All @@ -539,29 +587,45 @@

@open_pull_requests ||= {}
@open_pull_requests[cache_key] ||= begin
query = <<~EOS

Check warning on line 590 in Library/Homebrew/utils/github.rb

View check run for this annotation

Codecov / codecov/patch

Library/Homebrew/utils/github.rb#L590

Added line #L590 was not covered by tests
query($owner: String!, $repo: String!, $states: [PullRequestState!], $after: String) {
repository(owner: $owner, name: $repo) {
pullRequests(states: $states, first: 100, after: $after) {
nodes {
number
title
url
}
pageInfo {
hasNextPage
endCursor
}
}
}
}
EOS
owner, repo = tap_remote_repo.split("/")
endpoint = "repos/#{owner}/#{repo}/pulls"
query_parameters = ["state=open", "direction=desc"]
pull_requests = []
variables = { owner:, repo:, states: ["OPEN"] }
regex = pull_request_title_regex(name, version)

Check warning on line 609 in Library/Homebrew/utils/github.rb

View check run for this annotation

Codecov / codecov/patch

Library/Homebrew/utils/github.rb#L608-L609

Added lines #L608 - L609 were not covered by tests

API.paginate_rest("#{API_URL}/#{endpoint}", additional_query_params: query_parameters.join("&")) do |page|
pull_requests.concat(page)
pull_requests = []
API.paginate_graphql(query, variables:) do |result|
data = result.dig("repository", "pullRequests")
pull_requests.concat(data["nodes"])
data["pageInfo"]

Check warning on line 615 in Library/Homebrew/utils/github.rb

View check run for this annotation

Codecov / codecov/patch

Library/Homebrew/utils/github.rb#L611-L615

Added lines #L611 - L615 were not covered by tests
end

pull_requests
end

regex = pull_request_title_regex(name, version)
@open_pull_requests[cache_key].select { |pr| regex.match?(pr["title"]) }
.map { |pr| pr.merge("html_url" => pr.delete("url")) }

Check warning on line 621 in Library/Homebrew/utils/github.rb

View check run for this annotation

Codecov / codecov/patch

Library/Homebrew/utils/github.rb#L621

Added line #L621 was not covered by tests
rescue API::RateLimitExceededError => e
opoo e.message
pull_requests || []

Check warning on line 624 in Library/Homebrew/utils/github.rb

View check run for this annotation

Codecov / codecov/patch

Library/Homebrew/utils/github.rb#L623-L624

Added lines #L623 - L624 were not covered by tests
end

def self.check_for_duplicate_pull_requests(name, tap_remote_repo, state:, file:, quiet:, version: nil)
# `fetch_open_pull_requests` is more reliable but *really* slow, so let's use it only in CI.
pull_requests = if state == "open" && ENV["CI"].present?
fetch_open_pull_requests(name, tap_remote_repo, version:)
else
fetch_pull_requests(name, tap_remote_repo, state:, version:)
end
pull_requests = fetch_pull_requests(name, tap_remote_repo, state:, version:)

Check warning on line 628 in Library/Homebrew/utils/github.rb

View check run for this annotation

Codecov / codecov/patch

Library/Homebrew/utils/github.rb#L628

Added line #L628 was not covered by tests

pull_requests.select! do |pr|
get_pull_request_changed_files(
Expand Down
22 changes: 22 additions & 0 deletions Library/Homebrew/utils/github/api.rb
Original file line number Diff line number Diff line change
Expand Up @@ -318,6 +318,28 @@
end
end

sig {
params(

Check warning on line 322 in Library/Homebrew/utils/github/api.rb

View check run for this annotation

Codecov / codecov/patch

Library/Homebrew/utils/github/api.rb#L322

Added line #L322 was not covered by tests
query: String,
variables: T.nilable(T::Hash[Symbol, T.untyped]),
_block: T.proc.params(data: T::Hash[String, T.untyped]).returns(T::Hash[String, T.untyped]),
).void
}
def self.paginate_graphql(query, variables: nil, &_block)
result = API.open_graphql(query, variables:)

Check warning on line 329 in Library/Homebrew/utils/github/api.rb

View check run for this annotation

Codecov / codecov/patch

Library/Homebrew/utils/github/api.rb#L329

Added line #L329 was not covered by tests

has_next_page = T.let(true, T::Boolean)
variables ||= {}
while has_next_page

Check warning on line 333 in Library/Homebrew/utils/github/api.rb

View check run for this annotation

Codecov / codecov/patch

Library/Homebrew/utils/github/api.rb#L331-L333

Added lines #L331 - L333 were not covered by tests
page_info = yield result
has_next_page = page_info["hasNextPage"]

Check warning on line 335 in Library/Homebrew/utils/github/api.rb

View check run for this annotation

Codecov / codecov/patch

Library/Homebrew/utils/github/api.rb#L335

Added line #L335 was not covered by tests
if has_next_page
variables[:after] = page_info["endCursor"]
result = API.open_graphql(query, variables:)

Check warning on line 338 in Library/Homebrew/utils/github/api.rb

View check run for this annotation

Codecov / codecov/patch

Library/Homebrew/utils/github/api.rb#L338

Added line #L338 was not covered by tests
end
end
end

def self.raise_error(output, errors, http_code, headers, scopes)
json = begin
JSON.parse(output)
Expand Down