Skip to content

Commit

Permalink
Ensure CI workflow passes
Browse files Browse the repository at this point in the history
For a long time, GOV.UK had a convention whereby repositories had
to specify a status check called 'test', which should be the name
of the CI job that runs a repo's tests, linter, and so on.
These would all run under the 'test' job, usually via something
like `bundle exec rake`.

In January 2023, a [new convention](alphagov/content-data-admin@89a888b)
was established, whereby there would now be an overall workflow
called 'CI'. Within this workflow, there would be several individual
jobs, each with their own responsibility (e.g. 'test-ruby' for
running tests, 'lint-ruby' for running the linter, and so on).

We should therefore no longer look for a 'test' job: the modern
equivalent is to check for the 'CI' workflow, and to validate
that every job within the CI workflow passes.

Note that as a [stopgap](#21),
we temporarily stopped looking for a 'test' job and switched to
looking for a 'test-ruby' job instead. This is suboptimal as it
only checks for the outcome of the unit tests, and doesn't
consider other checks such as the linter, so it will incorrectly
[try and fail to approve and merge](alphagov/support-api#870 (review))
PRs whose unit tests pass but where there is another check failing.
  • Loading branch information
ChrisBAshton committed Dec 27, 2023
1 parent 10471d5 commit c59f288
Show file tree
Hide file tree
Showing 2 changed files with 111 additions and 24 deletions.
32 changes: 27 additions & 5 deletions lib/pull_request.rb
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,10 @@ def is_auto_mergeable?
reasons_not_to_merge << "PR contains more than one commit."
elsif !validate_files_changed
reasons_not_to_merge << "PR changes files that should not be changed."
elsif !validate_ci_workflow_exists
reasons_not_to_merge << "CI workflow doesn't exist."
elsif !validate_ci_passes
reasons_not_to_merge << "CI is failing or doesn't exist (should be a GitHub Action with a key called 'Test Ruby')."
reasons_not_to_merge << "CI workflow is failing."
elsif !validate_external_config_file
reasons_not_to_merge << "The remote .govuk_dependabot_merger.yml file is missing or in the wrong format."
else
Expand Down Expand Up @@ -56,14 +58,20 @@ def validate_files_changed
files_changed == ["Gemfile.lock"]
end

def validate_ci_workflow_exists
!ci_workflow_run_id.nil?
end

def validate_ci_passes
# No method exists for this in Octokit,
# so we need to make the API call manually.
uri = "https://api.github.com/repos/alphagov/#{@api_response.base.repo.name}/commits/#{@api_response.head.sha}/check-runs"
check_runs = HTTParty.get(uri)["check_runs"]
return false unless check_runs && (ci_run = check_runs.find { |run| run["name"] == "Test Ruby" })
jobs_url = "https://api.github.com/repos/alphagov/#{@api_response.base.repo.name}/actions/runs/#{ci_workflow_run_id}/jobs"
jobs = HTTParty.get(jobs_url)["jobs"]

unfinished_jobs = jobs.reject { |job| job["status"] == "completed" }
failed_jobs = jobs.reject { |job| job["conclusion"] == "success" }

ci_run["conclusion"] == "success"
unfinished_jobs.empty? && failed_jobs.empty?
end

def validate_external_config_file
Expand Down Expand Up @@ -146,4 +154,18 @@ def tell_dependency_manager_what_dependabot_is_changing
dependency_manager.add_dependency(name:, version:) if mentioned_dependencies[name]&.fetch(:to_version) == version
end
end

private

def ci_workflow_run_id
return @ci_workflow_run_id unless @ci_workflow_run_id.nil?

# No method exists for this in Octokit,
# so we need to make the API call manually.
ci_workflow_api_response = HTTParty.get("https://api.github.com/repos/alphagov/#{@api_response.base.repo.name}/actions/runs?head_sha=#{@api_response.head.sha}")
ci_workflow = ci_workflow_api_response["workflow_runs"].find { |run| run["name"] == "CI" }
return nil if ci_workflow.nil?

@ci_workflow_run_id = ci_workflow["id"]
end
end
103 changes: 84 additions & 19 deletions spec/lib/pull_request_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -219,13 +219,24 @@ def multiple_dependencies_commit
])
end

it "should make a call to validate_ci_workflow_exists" do
pr = create_pull_request_instance
allow(pr).to receive(:validate_ci_workflow_exists).and_return(false)
expect(pr).to receive(:validate_ci_workflow_exists)
pr.is_auto_mergeable?
expect(pr.reasons_not_to_merge).to eq([
"CI workflow doesn't exist.",
])
end

it "should make a call to validate_ci_passes" do
stub_successful_check_run
pr = create_pull_request_instance
allow(pr).to receive(:validate_ci_passes).and_return(false)
expect(pr).to receive(:validate_ci_passes)
pr.is_auto_mergeable?
expect(pr.reasons_not_to_merge).to eq([
"CI is failing or doesn't exist (should be a GitHub Action with a key called 'Test Ruby').",
"CI workflow is failing.",
])
end

Expand Down Expand Up @@ -295,19 +306,67 @@ def create_mock_dependency_manager
end
end

describe "#validate_ci_workflow_exists" do
it "returns true if there is a workflow named 'CI'" do
stub_ci_endpoint({
"workflow_runs": [
{ "name": "CI", "id": 1 },
],
})

pr = PullRequest.new(pull_request_api_response)
expect(pr.validate_ci_workflow_exists).to eq(true)
end

it "returns false if there is no workflow named 'CI'" do
stub_ci_endpoint({
"workflow_runs": [
{ "name": "foo", "id": 2 },
],
})

pr = PullRequest.new(pull_request_api_response)
expect(pr.validate_ci_workflow_exists).to eq(false)
end
end

describe "#validate_ci_passes" do
it "returns true if 'Test Ruby' status check passes'" do
stub_successful_check_run
let(:ci_workflow_id) { 1234 }
before do
stub_ci_endpoint({
"workflow_runs": [
{ "name": "CI", "id": ci_workflow_id },
],
})
end

it "returns true if all status checks in 'CI' workflow are passing'" do
stub_runs_endpoint(ci_workflow_id, {
"jobs": [
{ "status": "completed", "conclusion": "success" },
],
})

pr = PullRequest.new(pull_request_api_response)
expect(pr.validate_ci_passes).to eq(true)
end

it "returns false if 'Test Ruby' status check fails" do
stub_check_run({
name: "Test Ruby",
status: "completed",
conclusion: "failure",
it "returns false if any status check is still pending" do
stub_runs_endpoint(ci_workflow_id, {
"jobs": [
{ "status": "in_progress" },
],
})

pr = PullRequest.new(pull_request_api_response)
expect(pr.validate_ci_passes).to eq(false)
end

it "returns false if any status check failed" do
stub_runs_endpoint(ci_workflow_id, {
"jobs": [
{ "status": "completed", "conclusion": "failure" },
],
})

pr = PullRequest.new(pull_request_api_response)
Expand Down Expand Up @@ -470,21 +529,27 @@ def stub_remote_commit(head_commit_api_response)
end

def stub_successful_check_run
stub_check_run({
name: "Test Ruby",
status: "completed",
conclusion: "success",
ci_workflow_id = 123
stub_ci_endpoint({
"workflow_runs": [
{ "name": "CI", "id": ci_workflow_id },
],
})
stub_runs_endpoint(ci_workflow_id, {
"jobs": [
{ "status": "completed", "conclusion": "success" },
],
})
end

def stub_check_run(run)
check_run_api_response = {
"total_count": 1,
"check_runs": [run],
}
def stub_ci_endpoint(workflow_api_response)
stub_request(:get, "https://api.github.com/repos/alphagov/#{repo_name}/actions/runs?head_sha=#{sha}")
.to_return(status: 200, body: workflow_api_response.to_json, headers: { "Content-Type": "application/json" })
end

stub_request(:get, "https://api.github.com/repos/alphagov/#{repo_name}/commits/#{sha}/check-runs")
.to_return(status: 200, body: check_run_api_response.to_json, headers: { "Content-Type": "application/json" })
def stub_runs_endpoint(ci_workflow_id, ci_workflow_api_response)
stub_request(:get, "https://api.github.com/repos/alphagov/#{repo_name}/actions/runs/#{ci_workflow_id}/jobs")
.to_return(status: 200, body: ci_workflow_api_response.to_json, headers: { "Content-Type": "application/json" })
end
end

Expand Down

0 comments on commit c59f288

Please sign in to comment.