Skip to content

[release-1.7] ci: switch away from pull_request_target in the pre-commit workflow [RHIDP 11561] (#306) - #311

Merged
rm3l merged 1 commit into
redhat-developer:release-1.7from
rm3l:RHIDP-11561--update-gh-workflows--1.7
Jan 30, 2026
Merged

[release-1.7] ci: switch away from pull_request_target in the pre-commit workflow [RHIDP 11561] (#306)#311
rm3l merged 1 commit into
redhat-developer:release-1.7from
rm3l:RHIDP-11561--update-gh-workflows--1.7

Conversation

@rm3l

@rm3l rm3l commented Jan 30, 2026

Copy link
Copy Markdown
Member

Manual cherry-pick of #306

…ommit workflow [RHIDP 11561] (redhat-developer#306)

Co-authored-by: rhdh-qodo-merge[bot] <232573409+rhdh-qodo-merge[bot]@users.noreply.github.com>
@sonarqubecloud

Copy link
Copy Markdown

@rhdh-qodo-merge

Copy link
Copy Markdown

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🔒 No security concerns identified
⚡ Recommended focus areas for review

Permissions

The workflow posts and deletes PR comments via the Issues API (github.rest.issues.createComment, github.rest.issues.listComments, github.rest.issues.deleteComment), but the job only grants pull-requests: write. GitHub typically requires issues: write for these endpoints even when the target is a PR. Validate that the workflow can successfully list/create/delete comments with the current permissions; otherwise add issues: write (and keep permissions as narrow as possible).

permissions:
  pull-requests: write

steps:

  - name: Get the PR number from the workflow run
    id: pr-number
    uses: actions/github-script@ed597411d8f924073f98dfc5c65a23a2325f34cd # v8
    with:
      script: |
        const prs = context.payload.workflow_run.pull_requests;
        if (prs.length > 0) {
          const num = prs[0].number;
          if (Number.isInteger(num)) {
            core.setOutput('number', num);
          } else {
            core.setFailed(`Invalid PR number detected: ${num}`);
          }
        }

  - name: Delete previous pre-commit failure comments
    if: steps.pr-number.outputs.number
    uses: actions/github-script@ed597411d8f924073f98dfc5c65a23a2325f34cd # v8
    env:
      PR_NUMBER: ${{ steps.pr-number.outputs.number }}
    with:
      script: |
        const prNumber = parseInt(process.env.PR_NUMBER, 10);

        // Get all comments on the PR
        const { data: comments } = await github.rest.issues.listComments({
          owner: context.repo.owner,
          repo: context.repo.repo,
          issue_number: prNumber,
        });

        console.log(`Found ${comments.length} total comments on PR #${prNumber}`);

        const botComments = comments.filter(comment => {
          return comment.user.login === 'github-actions[bot]' && 
                 comment.body && 
                 comment.body.includes('⚠️ Pre-commit hook failures');
        });

        for (const comment of botComments) {
          try {
            await github.rest.issues.deleteComment({
              owner: context.repo.owner,
              repo: context.repo.repo,
              comment_id: comment.id,
            });
          } catch (error) {
            console.warn(`Failed to delete comment ${comment.id}:`, error.message);
CI behavior

Switching from pull_request_target to pull_request removes the ability to push auto-fixes back to forked PR branches, and the workflow now fails directly on hook failures (no continue-on-error). Confirm this is the intended UX for contributors (especially external forks) and that required checks / branch protection rules align with the new “comment-only” remediation flow.

  pull_request:
    branches:
      - main
      - release-1.[0-9]+

concurrency:
  group: ${{ github.workflow }}-${{ github.event.number }}
  cancel-in-progress: true

# Revoke all permissions by default, then grant only what is needed
permissions:
  contents: read

jobs:
  pre-commit:
    name: Pre-commit
    runs-on: ubuntu-latest
    env:
      GO111MODULE: on
    steps:
      - name: Checkout
        uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4
        with:
          fetch-depth: 0
          persist-credentials: false # Avoid token leakage to hooks

      - uses: actions/setup-python@a26af69be951a213d495a4c3e4e4022e16d87065 # v5
        with:
          python-version: 3.13

      - uses: actions/setup-go@d35c59abb061a4a6fb18e82ac0862c26744d6ab5 # v5
        with:
          go-version: ^1

      - name: Setup helm-docs
        run: go install github.com/norwoodj/helm-docs/cmd/helm-docs@latest

      - name: Run pre-commit
        uses: pre-commit/action@2c7b3805fd2a0fd8c1884dcaebf91fc102a13ecd # v3.0.1
        with:
          extra_args: --verbose --all-files --show-diff-on-failure
📄 References
  1. redhat-developer/rhdh-chart/ct.yaml [1-10]
  2. redhat-developer/rhdh-chart/ct-install.yaml [1-11]
  3. redhat-developer/rhdh-chart/cr.yaml [1-2]
  4. redhat-developer/rhdh-chart/charts/orchestrator-software-templates/ci/upstream-values.yaml [1-16]
  5. redhat-developer/rhdh-chart/charts/orchestrator-software-templates-infra/ci/upstream-values.yaml [1-27]
  6. redhat-developer/rhdh-chart/charts/orchestrator-software-templates-infra/templates/openshift-pipelines/subscription.yaml [0-2]
  7. redhat-developer/rhdh-chart/charts/orchestrator-software-templates-infra/templates/openshift-pipelines/post-cleanup.yaml [0-2]
  8. redhat-developer/rhdh-chart/charts/orchestrator-software-templates-infra/templates/openshift-gitops/subscription.yaml [0-2]

@rhdh-qodo-merge rhdh-qodo-merge Bot added the enhancement New feature or request label Jan 30, 2026
@rhdh-qodo-merge

Copy link
Copy Markdown

PR Type

Enhancement


Description

  • Switch pre-commit workflow from pull_request_target to pull_request trigger

  • Remove manual authorization safeguard and automatic commit/push functionality

  • Add new pre-commit-comment workflow to post failure notifications on PRs

  • Update pull request template with clearer pre-commit instructions

  • Simplify workflow permissions and improve security posture


File Walkthrough

Relevant files
Documentation
pull_request_template.md
Update pre-commit instructions in PR template                       

.github/pull_request_template.md

  • Updated pre-commit command from pre-commit run -a to pre-commit run
    --all-files
  • Changed workflow behavior description from automatic application to
    enforcement with warnings
  • Clarified that users should push resulting changes manually
+1/-1     
Enhancement
pre-commit-comment.yaml
New workflow for pre-commit failure notifications               

.github/workflows/pre-commit-comment.yaml

  • New workflow triggered by pre-commit workflow completion via
    workflow_run event
  • Extracts PR number from workflow run and deletes previous pre-commit
    failure comments
  • Posts detailed comment with instructions when pre-commit hooks fail
  • Provides setup and execution steps for fixing pre-commit issues
+103/-0 
pre-commit.yaml
Simplify pre-commit workflow and remove auto-commit logic

.github/workflows/pre-commit.yaml

  • Changed trigger from pull_request_target to pull_request for improved
    security
  • Removed authorize job that required manual approval for external fork
    PRs
  • Removed automatic commit and push functionality for pre-commit changes
  • Removed PR commenting logic (moved to separate pre-commit-comment
    workflow)
  • Simplified permissions to read-only at workflow level
  • Removed checkout of external fork repository and credentials handling
  • Removed continue-on-error flag and diff-checking logic
+6/-59   

@rm3l
rm3l merged commit d8424f6 into redhat-developer:release-1.7 Jan 30, 2026
6 checks passed
@rm3l
rm3l deleted the RHIDP-11561--update-gh-workflows--1.7 branch January 30, 2026 16:51
@rhdh-qodo-merge

Copy link
Copy Markdown

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Checkout explicit PR head SHA

In the pre-commit workflow, explicitly check out the pull request's head SHA by
adding ref: ${{ github.event.pull_request.head.sha }} to the checkout step.

.github/workflows/pre-commit.yaml [24-28]

 - name: Checkout
   uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4
   with:
     fetch-depth: 0
+    ref: ${{ github.event.pull_request.head.sha }}
     persist-credentials: false # Avoid token leakage to hooks
  • Apply / Chat
Suggestion importance[1-10]: 8

__

Why: This is a critical improvement that ensures the pre-commit hooks run on the exact code from the PR, which was the behavior of the removed code and is essential for correctness.

Medium
Only delete comments on workflow failure

Add a condition to the 'Delete previous pre-commit failure comments' step to
only run it if the current workflow run has failed.

.github/workflows/pre-commit-comment.yaml [34-68]

 - name: Delete previous pre-commit failure comments
-  if: steps.pr-number.outputs.number
+  if: steps.pr-number.outputs.number && github.event.workflow_run.conclusion == 'failure'
   uses: actions/github-script@ed597411d8f924073f98dfc5c65a23a2325f34cd # v8
   env:
     PR_NUMBER: ${{ steps.pr-number.outputs.number }}
   with:
     script: |
       const prNumber = parseInt(process.env.PR_NUMBER, 10);
 
       // Get all comments on the PR
       const { data: comments } = await github.rest.issues.listComments({
         owner: context.repo.owner,
         repo: context.repo.repo,
         issue_number: prNumber,
       });
 
       console.log(`Found ${comments.length} total comments on PR #${prNumber}`);
 
       const botComments = comments.filter(comment => {
         return comment.user.login === 'github-actions[bot]' && 
                comment.body && 
                comment.body.includes('⚠️ Pre-commit hook failures');
       });
 
       for (const comment of botComments) {
         try {
           await github.rest.issues.deleteComment({
             owner: context.repo.owner,
             repo: context.repo.repo,
             comment_id: comment.id,
           });
         } catch (error) {
           console.warn(`Failed to delete comment ${comment.id}:`, error.message);
         }
       }
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: This is a good suggestion that improves the workflow's logic by only deleting old failure comments when a new failure occurs, which is more intuitive behavior.

Medium
Restrict comment deletion pattern

Change the comment filter from comment.body.includes('⚠️ Pre-commit hook
failures') to comment.body.startsWith('## ⚠️ Pre-commit hook failures') to make
the matching more specific.

.github/workflows/pre-commit-comment.yaml [52-56]

 const botComments = comments.filter(comment => {
   return comment.user.login === 'github-actions[bot]' && 
          comment.body && 
-         comment.body.includes('⚠️ Pre-commit hook failures');
+         comment.body.startsWith('## ⚠️ Pre-commit hook failures');
 });
  • Apply / Chat
Suggestion importance[1-10]: 5

__

Why: This suggestion improves the robustness of the comment deletion logic by making the search pattern more specific, reducing the risk of accidentally deleting unrelated comments.

Low
Possible issue
Fail if no PR found

In the script to get the PR number, add a check to fail the job if the
pull_requests array is empty.

.github/workflows/pre-commit-comment.yaml [24-26]

 const prs = context.payload.workflow_run.pull_requests;
-if (prs.length > 0) {
-  const num = prs[0].number;
+if (prs.length === 0) {
+  core.setFailed('No pull request associated with this workflow run');
+  return;
+}
+const num = prs[0].number;
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why: This suggestion correctly identifies an edge case where no PR is found and improves the workflow's robustness by explicitly failing instead of silently skipping steps.

Low
  • More

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request Review effort 2/5

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant