diff --git a/data/sync/configs/20261014-all-checks-job.yaml b/data/sync/configs/20261014-all-checks-job.yaml new file mode 100644 index 0000000..93a3491 --- /dev/null +++ b/data/sync/configs/20261014-all-checks-job.yaml @@ -0,0 +1,62 @@ +--- +# Add the 'All checks' aggregate job to the pupmod PR Tests workflow. +# +# GitHub only blocks a merge on status checks that branch protection lists +# by name, so a job can fail without stopping a merge. The new `all-checks` +# job needs every other job in pr_tests.yml and fails unless all of them +# succeeded, giving branch protection one check to require in every repo. +# +# pr_tests.yml (merge_github_workflows): the repo's own `jobs.acceptance` +# block is kept as-is. A module with no acceptance job keeps having none, +# and the merge drops `acceptance` from all-checks' `needs:` there. +# +# Run this after the 20261003-gate-ruby-style PRs merge. Before that, these +# PRs would also carry the Ruby Style gating change. +# +# After the PRs merge, require the 'All checks' status check in each repo's +# branch protection. Requiring it before the job exists leaves every PR +# waiting on a check that never reports. +puppetsync::plan_config: + puppetsync: + permitted_project_types: + - pupmod + - pupmod_skeleton + plans: + sync: + github_api_delay_seconds: 1 + merge_github_workflows: + files: + - pr_tests.yml + preserve_blocks: + - jobs.acceptance + stages: + - install_gems + - checkout_git_feature_branch_in_each_repo + - merge_github_workflows + - git_commit_changes + - ensure_github_fork + - ensure_git_remote + - git_push_to_remote + - ensure_github_pr + + approve_github_prs: + stages: + - approve_github_pr_for_each_repo + + merge_github_prs: + stages: + - merge_github_pr_for_each_repo + + git: + feature_branch: puppetsync/20261014-all-checks-job + commit_message: | + [puppetsync] Add an 'All checks' job to PR Tests + + GitHub only blocks a merge on status checks that branch protection + lists by name. This patch adds an `all-checks` job that needs every + other PR Tests job and fails unless all of them succeeded, so branch + protection can require that single check. + + github: + pr_user: silug # This should be the account that *submitted* the PRs + approval_message: ':+1: :ghost:' diff --git a/data/sync/configs/latest.yaml b/data/sync/configs/latest.yaml index 772d90b..fd33f65 120000 --- a/data/sync/configs/latest.yaml +++ b/data/sync/configs/latest.yaml @@ -1 +1 @@ -20261002-openvox9-ga.yaml \ No newline at end of file +20261014-all-checks-job.yaml \ No newline at end of file diff --git a/dist/puppetsync/tasks/merge_gha_workflows.rb b/dist/puppetsync/tasks/merge_gha_workflows.rb index 98a6a40..3d82bf2 100644 --- a/dist/puppetsync/tasks/merge_gha_workflows.rb +++ b/dist/puppetsync/tasks/merge_gha_workflows.rb @@ -28,7 +28,10 @@ # block it is appended to the parent; and when the EXISTING file lacks it, # the template's block is REMOVED — the template must not introduce a # repo-owned section (an acceptance job would be dead weight on a module -# with no acceptance suites). +# with no acceptance suites). A removed job is also dropped from every +# other job's `needs:`, since GitHub rejects a workflow whose `needs:` +# names a job that doesn't exist (the template's aggregate check job +# needs every job, acceptance included). require 'fileutils' require 'json' @@ -105,6 +108,7 @@ def node_at_path(root, path) # key.start_line ... value.end_line, exclusive) def preserve_existing_blocks(template, existing_text, paths) preserved = [] + removed = [] existing_lines = existing_text.split("\n", -1) existing_root = root_mapping(existing_text) @@ -120,6 +124,7 @@ def preserve_existing_blocks(template, existing_text, paths) next if template_pair.nil? template_lines[template_pair[0].start_line...template_pair[1].end_line] = [] + removed << path elsif template_pair block = existing_lines[existing_pair[0].start_line...existing_pair[1].end_line] next if template_lines[template_pair[0].start_line...template_pair[1].end_line] == block @@ -137,11 +142,52 @@ def preserve_existing_blocks(template, existing_text, paths) template = template_lines.join("\n") end - [template, preserved] + [template, preserved, removed] +end + +# Drop the named jobs from every job's `needs:` (a scalar, a flow sequence +# on one line, or a block sequence with one item per line), removing the +# `needs:` key entirely when nothing is left. One edit per pass, re-parsing +# after each so line/column positions stay valid. +def prune_needs(template, job_names) + return template if job_names.empty? + + loop do + jobs = (root = root_mapping(template)) && node_at_path(root, 'jobs') + return template unless jobs && jobs[1].is_a?(Psych::Nodes::Mapping) + + lines = template.split("\n", -1) + edit = jobs[1].children.each_slice(2).lazy.map { |_name, job| + needs = job.is_a?(Psych::Nodes::Mapping) && node_at_path(job, 'needs') + next unless needs + + key, value = needs + names = value.is_a?(Psych::Nodes::Sequence) ? value.children.map(&:value) : [value.value] + kept = names - job_names + next if kept == names + + if kept.empty? + # A one-line value ends on the key's own line + lines[key.start_line...[value.end_line, key.start_line + 1].max] = [] + elsif value.is_a?(Psych::Nodes::Sequence) && value.style == Psych::Nodes::Sequence::BLOCK + drop = value.children.reject { |item| kept.include?(item.value) }.map(&:start_line) + lines = lines.reject.with_index { |_l, i| drop.include?(i) } + else + line = lines[value.start_line] + lines[value.start_line] = line[0...value.start_column] + "[#{kept.join(', ')}]" + line[value.end_column..] + end + lines.join("\n") + }.find(&:itself) + return template if edit.nil? || edit == template + + template = edit + end end def merge_workflow(template, existing_text, keys, blocks) - template, preserved_blocks = preserve_existing_blocks(template, existing_text, blocks) + template, preserved_blocks, removed_blocks = preserve_existing_blocks(template, existing_text, blocks) + removed_jobs = removed_blocks.filter_map { |path| path.delete_prefix('jobs.') if path.match?(/\Ajobs\.[^.]+\z/) } + template = prune_needs(template, removed_jobs) existing = existing_values(existing_text, keys) lines = template.split("\n", -1) updated = [] diff --git a/modules/profile/files/pupmod/_github/workflows/pr_tests.simp-simp.yml b/modules/profile/files/pupmod/_github/workflows/pr_tests.simp-simp.yml index 81716ba..a3591a1 100644 --- a/modules/profile/files/pupmod/_github/workflows/pr_tests.simp-simp.yml +++ b/modules/profile/files/pupmod/_github/workflows/pr_tests.simp-simp.yml @@ -150,6 +150,32 @@ jobs: - run: ${{matrix.test_suite.run}} continue-on-error: ${{matrix.puppet.experimental}} + # The one check for branch protection to require: GitHub only blocks a + # merge on checks listed by name, and this job fails unless every job it + # needs succeeded, so the required list never has to track job or matrix + # names. Add new jobs to `needs:`; puppetsync drops a job a repo doesn't + # have (e.g. acceptance) from the list. + all-checks: + name: 'All checks' + if: always() + needs: + - puppet-syntax + - puppet-style + - ruby-style + - file-checks + - reference + - releng-checks + - spec-tests + - acceptance + runs-on: ubuntu-latest + steps: + - name: 'Fail unless every needed job succeeded' + env: + NEEDS: ${{ toJSON(needs) }} + run: | + jq -r 'to_entries[] | "\(.key): \(.value.result)"' <<< "$NEEDS" + jq -e 'all(.[]; .result == "success")' <<< "$NEEDS" > /dev/null + acceptance: runs-on: - ubuntu-latest diff --git a/modules/profile/files/pupmod/_github/workflows/pr_tests.yml b/modules/profile/files/pupmod/_github/workflows/pr_tests.yml index 2bdcbe0..ce122a7 100644 --- a/modules/profile/files/pupmod/_github/workflows/pr_tests.yml +++ b/modules/profile/files/pupmod/_github/workflows/pr_tests.yml @@ -134,6 +134,32 @@ jobs: - run: 'bundle exec rake parallel_spec' continue-on-error: ${{matrix.puppet.experimental}} + # The one check for branch protection to require: GitHub only blocks a + # merge on checks listed by name, and this job fails unless every job it + # needs succeeded, so the required list never has to track job or matrix + # names. Add new jobs to `needs:`; puppetsync drops a job a repo doesn't + # have (e.g. acceptance) from the list. + all-checks: + name: 'All checks' + if: always() + needs: + - puppet-syntax + - puppet-style + - ruby-style + - file-checks + - reference + - releng-checks + - spec-tests + - acceptance + runs-on: ubuntu-latest + steps: + - name: 'Fail unless every needed job succeeded' + env: + NEEDS: ${{ toJSON(needs) }} + run: | + jq -r 'to_entries[] | "\(.key): \(.value.result)"' <<< "$NEEDS" + jq -e 'all(.[]; .result == "success")' <<< "$NEEDS" > /dev/null + acceptance: runs-on: - ubuntu-latest diff --git a/spec/tasks/merge_gha_workflows_spec.rb b/spec/tasks/merge_gha_workflows_spec.rb index 7ea2425..869f11f 100644 --- a/spec/tasks/merge_gha_workflows_spec.rb +++ b/spec/tasks/merge_gha_workflows_spec.rb @@ -359,6 +359,56 @@ def run_merge(workflows, preserve_blocks: nil) expect(JSON.parse(stdout)['changed']).to be false end + it "drops a removed job from other jobs' needs" do + with_needs = template.sub(" acceptance:\n", <<~YAML.gsub(/^/, ' ') + " acceptance:\n") + flow: + needs: [spec, acceptance] + block: + needs: + - spec + - acceptance + only: + needs: acceptance + only_list: + needs: [acceptance] + YAML + File.write(@wf, "name: PR Tests\njobs:\n spec:\n steps:\n - uses: actions/checkout@v5\n") + + stdout, stderr, status = run_merge([{ 'path' => @wf, 'template' => with_needs }], + preserve_blocks: ['jobs.acceptance']) + + expect(status).to be_success, stderr + jobs = Psych.safe_load(File.read(@wf))['jobs'] + expect(jobs.keys).to eq(%w[spec flow block only only_list]) + expect(jobs['flow']).to eq('needs' => ['spec']) + expect(jobs['block']).to eq('needs' => ['spec']) + expect(jobs['only']).to be_nil # `needs:` removed; the job is now empty + expect(jobs['only_list']).to be_nil + end + + it "keeps a needs: entry for a job the repo has" do + File.write(@wf, real_template) + + stdout, stderr, status = run_merge([{ 'path' => @wf, 'template' => real_template }], + preserve_blocks: ['jobs.acceptance']) + + expect(status).to be_success, stderr + expect(JSON.parse(stdout)['changed']).to be false + expect(Psych.safe_load(File.read(@wf))['jobs']['all-checks']['needs']).to include('acceptance') + end + + it 'gives a module without acceptance suites a valid aggregate job' do + File.write(@wf, real_template.sub(/^ acceptance:.*\z/m, '')) + + stdout, stderr, status = run_merge([{ 'path' => @wf, 'template' => real_template }], + preserve_blocks: ['jobs.acceptance']) + + expect(status).to be_success, stderr + jobs = Psych.safe_load(File.read(@wf))['jobs'] + expect(jobs.keys).not_to include('acceptance') + expect(jobs['all-checks']['needs']).to eq(jobs.keys - ['all-checks']) + end + it 'is idempotent' do existing = template .sub('name: PR Tests', 'name: Old Name')