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

AutoMerge: Allow skipping sequential versions with author-approval criteria #539

Merged
merged 16 commits into from
Jan 30, 2024
Merged
Show file tree
Hide file tree
Changes from 7 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: 4 additions & 0 deletions docs/src/guidelines.md
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ function guidelines_to_markdown_output(guidelines_function::Function)
check_license = true,
this_is_jll_package = false,
this_pr_can_use_special_jll_exceptions = false,
use_distance_check = false,
author_approved = false
ericphanson marked this conversation as resolved.
Show resolved Hide resolved
)
filter!(x -> x[1] != :update_status, guidelines)
filter!(x -> !(x[1].docs isa Nothing), guidelines)
Expand Down Expand Up @@ -50,6 +52,8 @@ function guidelines_to_markdown_output(guidelines_function::Function)
check_license = true,
this_is_jll_package = false,
this_pr_can_use_special_jll_exceptions = false,
use_distance_check = false,
author_approved = false
ericphanson marked this conversation as resolved.
Show resolved Hide resolved
)
filter!(x -> x[1] != :update_status, guidelines)
filter!(x -> !(x[1].docs isa Nothing), guidelines)
Expand Down
4 changes: 4 additions & 0 deletions docs/src/private-registries.md
Original file line number Diff line number Diff line change
Expand Up @@ -99,3 +99,7 @@ In that case you will also have to add the following
+ - run: chmod +x .ci/instantiate.sh
```
in `ci.yml` and also `TagBotTriggers.yml` and `automerge.yml` (in which the above appears twice) files if those features are used.

## Author approval workflow support

Some guidelines allow the person invoking registration (typically the package author) to "approve" AutoMerge even if the guideline is not passing. This is facilitated by a labelling workflow `author_approval.yml` that must run on the registry in order to translate author-approval comments into labels that AutoMerge can use. The [General registry's workflows](https://github.com/JuliaRegistries/General/tree/master/.github/workflows) should once again be used as an example.
7 changes: 7 additions & 0 deletions example_github_workflow_files/automerge.yml
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,17 @@ on:
schedule:
- cron: '05,17,29,41,53 * * * *'
pull_request:
# Here we run when labels are applied, so that the "Override AutoMerge: name similarity is okay label is respected.
types: [labeled, synchronize]
workflow_dispatch:

jobs:
AutoMerge:
# Run if the we are not triggered by a label OR we are triggered by a label, and that
# label is one that affects the execution of the workflow
# Note: since the label contains a colon, we need to use a workaround like https://github.com/actions/runner/issues/1019#issuecomment-810482716
# for the syntax to parse correctly.
if: "${{ github.event.action != 'labeled' || (github.event.action == 'labeled' && (github.event.label.name == 'Override AutoMerge: name similarity is okay' || github.event.label.name == 'package-author-approved')) }}"
ericphanson marked this conversation as resolved.
Show resolved Hide resolved
runs-on: ${{ matrix.os }}
strategy:
matrix:
Expand Down
6 changes: 5 additions & 1 deletion src/AutoMerge/cron.jl
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,11 @@ function all_specified_check_runs_passed(
return all(values(check_passed))
end

pr_comment_is_blocking(c::GitHub.Comment) = !occursin("[noblock]", body(c))
function pr_comment_is_blocking(c::GitHub.Comment)
# Note: `merge_approved` is not case sensitive, to match the semantics of `contains` on GHA
ericphanson marked this conversation as resolved.
Show resolved Hide resolved
not_blocking = occursin("[noblock]", body(c)) || occursin("merge approved", lowercase(body(c)))
Copy link
Member

Choose a reason for hiding this comment

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

I'm not a fan of this special casing. I'd rather just tell users to always include [noblock] in all of their comments.

Copy link
Member

Choose a reason for hiding this comment

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

As discussed in JuliaRegistries/General#98815, we're going to compromise, and we'll require [merge approved], but not require [noblock].

ericphanson marked this conversation as resolved.
Show resolved Hide resolved
return !not_blocking
end

function pr_has_no_blocking_comments(
api::GitHub.GitHubAPI,
Expand Down
33 changes: 31 additions & 2 deletions src/AutoMerge/guidelines.jl
Original file line number Diff line number Diff line change
Expand Up @@ -265,6 +265,8 @@ To prevent confusion between similarly named packages, the names of new packages
[VisualStringDistances.jl](https://github.com/ericphanson/VisualStringDistances.jl)
between the package name and any existing package must exceeds a certain
a hand-chosen threshold (currently 2.5).

These checks can be overridden by applying a label `Override AutoMerge: name similarity is okay` to the PR. This will turn off the check as long as the label is applied to the PR.
""",
check=data -> meets_distance_check(data.pkg, data.registry_master),
)
Expand Down Expand Up @@ -373,6 +375,24 @@ function meets_distance_check(
return (false, message)
end

# Used in `pull_request_build` to determine if we should
# perform the distance check or not.
# We expect to be passed the `labels` field of a PullRequest:
# <https://github.com/JuliaWeb/GitHub.jl/blob/d24bd6798609ae356db308d65577e99aad0cf432/src/issues/pull_requests.jl#L33>
function perform_distance_check(labels)
# No labels? Do the check
isnothing(labels) && return true
for label in labels
if label.name === "Override AutoMerge: name similarity is okay"
# found the override! Skip the check
@debug "Found label; skipping distance check" label.name
return false
end
end
# Did not find the override. Perform the check.
return true
end

const guideline_normal_capitalization = Guideline(;
info="Normal capitalization",
docs=string(
Expand Down Expand Up @@ -441,6 +461,10 @@ function _valid_change(old_version::VersionNumber, new_version::VersionNumber)
end
end

const AUTHOR_APPROVAL_INSTRUCTIONS = string(
ericphanson marked this conversation as resolved.
Show resolved Hide resolved
"**If this was not a mistake and you wish to merge this PR anyway,",
"write a comment that says \"merge approved\".**")
DilumAluthge marked this conversation as resolved.
Show resolved Hide resolved
ericphanson marked this conversation as resolved.
Show resolved Hide resolved

const guideline_sequential_version_number = Guideline(;
info="Sequential version number",
docs=string(
Expand All @@ -451,6 +475,7 @@ const guideline_sequential_version_number = Guideline(;
"valid new versions are `1.0.1`, `1.1.1`, `1.2.0` and `2.0.0`. ",
"Invalid new versions include `1.0.2` (skips `1.0.1`), ",
"`1.3.0` (skips `1.2.0`), `3.0.0` (skips `2.0.0`) etc.",
AUTHOR_APPROVAL_INSTRUCTIONS
ericphanson marked this conversation as resolved.
Show resolved Hide resolved
),
check=data -> meets_sequential_version_number(
data.pkg,
Expand Down Expand Up @@ -956,6 +981,8 @@ function get_automerge_guidelines(
check_license::Bool,
this_is_jll_package::Bool,
this_pr_can_use_special_jll_exceptions::Bool,
use_distance_check::Bool,
author_approved::Bool # currently unused for new packages
ericphanson marked this conversation as resolved.
Show resolved Hide resolved
)
guidelines = [
(guideline_registry_consistency_tests_pass, true),
Expand Down Expand Up @@ -987,7 +1014,7 @@ function get_automerge_guidelines(
# prints the list of similar package names in
# the automerge comment. To make the comment easy
# to read, we want this list to be at the end.
(guideline_distance_check, true),
(guideline_distance_check, use_distance_check),
]
return guidelines
end
Expand All @@ -997,11 +1024,13 @@ function get_automerge_guidelines(
check_license::Bool,
this_is_jll_package::Bool,
this_pr_can_use_special_jll_exceptions::Bool,
use_distance_check::Bool, # unused for new versions
author_approved::Bool
ericphanson marked this conversation as resolved.
Show resolved Hide resolved
)
guidelines = [
(guideline_registry_consistency_tests_pass, true),
(guideline_pr_only_changes_allowed_files, true),
(guideline_sequential_version_number, !this_pr_can_use_special_jll_exceptions),
(guideline_sequential_version_number, !this_pr_can_use_special_jll_exceptions && !author_approved),
ericphanson marked this conversation as resolved.
Show resolved Hide resolved
(guideline_version_number_no_prerelease, true),
(guideline_version_number_no_build, !this_pr_can_use_special_jll_exceptions),
(guideline_compat_for_julia, true),
Expand Down
2 changes: 2 additions & 0 deletions src/AutoMerge/pull_requests.jl
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,8 @@ function pull_request_build(data::GitHubAutoMergeData; check_license)::Nothing
check_license=check_license,
this_is_jll_package=this_is_jll_package,
this_pr_can_use_special_jll_exceptions=this_pr_can_use_special_jll_exceptions,
use_distance_check=perform_distance_check(data.pr.labels),
author_approved=has_author_approved_label(data.pr.labels)
ericphanson marked this conversation as resolved.
Show resolved Hide resolved
)
checked_guidelines = Guideline[]

Expand Down
16 changes: 16 additions & 0 deletions src/AutoMerge/util.jl
Original file line number Diff line number Diff line change
Expand Up @@ -288,3 +288,19 @@ function get_all_non_jll_package_names(registry_dir::AbstractString)
unique!(packages)
return packages
end

const AUTHOR_APPROVED_LABEL = "package-author-approved"
ericphanson marked this conversation as resolved.
Show resolved Hide resolved

function has_author_approved_label(labels)
ericphanson marked this conversation as resolved.
Show resolved Hide resolved
# No labels? Not approved
isnothing(labels) && return false
for label in labels
if label.name === AUTHOR_APPROVED_LABEL
ericphanson marked this conversation as resolved.
Show resolved Hide resolved
# found the approval
@debug "Found `$(AUTHOR_APPROVED_LABEL)` label"
ericphanson marked this conversation as resolved.
Show resolved Hide resolved
return true
end
end
# Did not find approval
return false
end
25 changes: 22 additions & 3 deletions test/automerge-unit.jl
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,25 @@ end
"ReallyLooooongNameCD", ["ReallyLooooongNameAB"]
)[1]
end
@testset "perform_distance_check" begin
@test AutoMerge.perform_distance_check(nothing)
@test AutoMerge.perform_distance_check([GitHub.Label(; name="hi")])
@test !AutoMerge.perform_distance_check([GitHub.Label(; name="Override AutoMerge: name similarity is okay")])
@test !AutoMerge.perform_distance_check([GitHub.Label(; name="hi"), GitHub.Label(; name="Override AutoMerge: name similarity is okay")])
end
@testset "has_author_approved_label" begin
@test !AutoMerge.has_author_approved_label(nothing)
ericphanson marked this conversation as resolved.
Show resolved Hide resolved
@test !AutoMerge.has_author_approved_label([GitHub.Label(; name="hi")])
ericphanson marked this conversation as resolved.
Show resolved Hide resolved
@test AutoMerge.has_author_approved_label([GitHub.Label(; name="package-author-approved")])
ericphanson marked this conversation as resolved.
Show resolved Hide resolved
@test AutoMerge.has_author_approved_label([GitHub.Label(; name="hi"), GitHub.Label(; name="package-author-approved")])
ericphanson marked this conversation as resolved.
Show resolved Hide resolved
end
@testset "pr_comment_is_blocking" begin
@test AutoMerge.pr_comment_is_blocking(GitHub.Comment(; body="hi"))
@test AutoMerge.pr_comment_is_blocking(GitHub.Comment(; body="block"))
@test !AutoMerge.pr_comment_is_blocking(GitHub.Comment(; body="[noblock]"))
@test !AutoMerge.pr_comment_is_blocking(GitHub.Comment(; body="[noblock]hi"))
@test !AutoMerge.pr_comment_is_blocking(GitHub.Comment(; body="merge approved abc"))
ericphanson marked this conversation as resolved.
Show resolved Hide resolved
end
@testset "`get_all_non_jll_package_names`" begin
registry_path = joinpath(DEPOT_PATH[1], "registries", "General")
packages = AutoMerge.get_all_non_jll_package_names(registry_path)
Expand Down Expand Up @@ -617,7 +636,7 @@ end
@test result[1]
result = has_osi_license_in_depot("VisualStringDistances")
@test result[1]

# Now, what happens if there's also a non-OSI license in another file?
pkg_path = pkgdir_from_depot(tmp_depot, "UnbalancedOptimalTransport")
open(joinpath(pkg_path, "LICENSE2"); write=true) do io
Expand All @@ -627,12 +646,12 @@ end
end
result = has_osi_license_in_depot("UnbalancedOptimalTransport")
@test result[1]

# What if we also remove the original license, leaving only the CC0 license?
rm(joinpath(pkg_path, "LICENSE"))
result = has_osi_license_in_depot("UnbalancedOptimalTransport")
@test !result[1]

# What about no license at all?
pkg_path = pkgdir_from_depot(tmp_depot, "VisualStringDistances")
rm(joinpath(pkg_path, "LICENSE"))
Expand Down
Loading