Skip to content

ci: align pre-commit hooks and GitHub templates - #517

Closed
mc-nv wants to merge 17 commits into
mainfrom
mchornyi/TRI-1100/github-align-hooks-and-templates
Closed

ci: align pre-commit hooks and GitHub templates#517
mc-nv wants to merge 17 commits into
mainfrom
mchornyi/TRI-1100/github-align-hooks-and-templates

Conversation

@mc-nv

@mc-nv mc-nv commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

What does the PR do?

Aligns this repository with the org-wide setup consolidated in triton-inference-server/.github:

  • .pre-commit-config.yaml: shared baseline hooks, conventional-commit message validation (commit-msg stage), and the centralized add-license hook from the org .github repository (rev: v1.4.3 — excludes .github/ templates, never rewrites LICENSE files).
  • Pre-commit CI runs only on files modified by the PR.
  • Caller stub for the org-wide reusable conventional-pr workflow (@v1.4.3): validates the PR title against Conventional Commits (hard gate — it becomes the squash-merge commit), derives one human-readable label per distinct type found in the title and all conforming commit subjects (e.g. ci:CI/CD, feat:feature, fix:fix), enforces org-wide label colors/descriptions, detects cherry-picks, and fails if no type is derivable and no type label is assigned.
  • No per-repo templates: PR templates and issue routing are inherited from the org-wide defaults in triton-inference-server/.github (issues route to the server repository).

Depends on triton-inference-server/.github#5 (pinned tags current: v1.4.3 — already exist, CI is green).

Pros / Cons

Pros

  • Single home (org .github repo) for hooks, templates, issue routing, and the PR-title workflow — one change propagates everywhere.
  • Repos carry only a config file and two small workflow stubs; no template copies to drift.
  • Commit/PR title format enforced both locally (commit-msg hook) and in CI (reusable workflow), with automatic type labels.
  • License hooks never modify LICENSE files.

Cons / risks

  • Version-pinned dependency on the org .github repository (tags are write-once; changes ship as a new tag + rev bump).
  • Template inheritance requires the org .github repository to remain public.

Related Issues / PRs

  • Resolves: TRI-1100

Related PRs:

Test plan

  • pre-commit validate-config passes; pre-commit run --files <PR diff> passes locally with the centralized hooks pinned to the .github branch SHA.
  • After .github#5 merges and v1.4.3 exists: the conventional-pr check validates this PR's own title and applies the ci label.
  • No LICENSE file content is changed by this PR.

Caveats

  • None beyond the merge-order note above.

Checklist

  • PR title follows <commit_type>: <Title> (conventional commit)
  • I ran pre-commit locally on all files changed by this PR and it passes
  • Copyright header is correct on all changed files
  • External contributors: I have read the Contribution guidelines and signed the Contributor License Agreement

Adopt the shared pre-commit baseline: two-line SPDX header, conventional
commit message validation (commit-msg stage), and the centralized
add-license / add-spdx-license hooks from developer_tools v0.2.0.
Run pre-commit CI only on files modified by the PR, and roll out the
standard issue templates and the simplified single PR template.

TRI-1100
@mc-nv mc-nv self-assigned this Jul 18, 2026
This was referenced Jul 18, 2026
Legal's Copyright / License Header Guidance specifies the SPDX form
without a comma after the year.

TRI-1100
@mc-nv
mc-nv marked this pull request as ready for review July 18, 2026 02:04
Human-readable type labels with enforced descriptions and colors.

TRI-1100
@github-actions github-actions Bot added CI/CD Continuous integration and workflow changes (ci: PRs) and removed ci labels Jul 20, 2026
@github-actions github-actions Bot added the chore Maintenance work, no production code change (chore: PRs) label Jul 20, 2026
Comment thread .github/workflows/pre-commit.yml Outdated
The add-license hook now fails when the LICENSE copyright year is
stale.

TRI-1100
@mc-nv
mc-nv requested a review from yinggeh July 21, 2026 15:14
Workflow files are license-processed again (only templates excluded);
refresh the stale copyright year this repo's pre-commit workflow
carried.

TRI-1100
@mc-nv

mc-nv commented Jul 21, 2026

Copy link
Copy Markdown
Contributor Author

@yinggeh — all your questions across the PR set are answered now; summary of where:

  • Commit-msg hook vs PR titles → developer_tools#61 thread: titles are CI-gated by the org-wide conventional-pr workflow; the commit-msg hook is local fast feedback and feeds multi-type labels. Happy to drop it if you prefer title-only.
  • When do LICENSE copyrights get updated → developer_tools#61 / .github#5 threads: manually and deliberately; since v1.4.0 the hook fails on a stale LICENSE year instead of rewriting it (four repos got deliberate year-bump commits).
  • Issue disabling → onnxruntime_backend#351 thread: only new-issue creation redirects to server; existing issues stay; migration offered as follow-up.
  • "No-op" workflow change → identity_backend#36 thread: it's the header-year maintenance, now enforced.
  • Dropped .patch exclude → third_party#80: real regression, great catch — restored as a top-level exclude in 48b35d8.
  • Commit type determination → server#8890 thread: title token + conforming commit subjects.

This PR (core) carries no repo-specific deviations — shared baseline only. Could you re-review when you get a chance?

Grant issues:write to the labeling job (review feedback).

TRI-1100
@mc-nv
mc-nv requested a review from pskiran1 July 22, 2026 15:11
@Vinya567

Copy link
Copy Markdown
Contributor

@greptileai

@greptile-apps

greptile-apps Bot commented Jul 22, 2026

Copy link
Copy Markdown

Greptile Summary

This PR aligns the repo's CI toolchain with the org-wide baseline: a new conventional-pr.yml caller workflow delegates PR-title validation and label management to the org-wide reusable workflow (v1.4.3), a reworked pre-commit.yml replaces the old pre-commit/action invocation with a manual install + null-delimited xargs pipeline for paths with spaces and a dedicated cache step, and .pre-commit-config.yaml gains a commit-msg-stage conventional-commit hook and switches the add-license hook to the centralized .github repository.

  • conventional-pr.yml: New caller stub with a guarded dual-event trigger that correctly routes same-repo PRs through pull_request (no write token needed) and fork PRs through pull_request_target (write token, safe because the reusable workflow never checks out PR code).
  • pre-commit.yml: Replaces the previous get modified files → pre-commit/action pattern with --diff-filter=d + null-delimited xargs -0 --no-run-if-empty, correctly filtering deleted files and handling spaces in paths; adds an actions/cache step to persist the pre-commit virtualenv between runs.
  • .pre-commit-config.yaml: Adds default_install_hook_types, a commit-msg-stage hook for conventional-commit format enforcement, fixes flake8 arg quoting, and bumps add-license to the centralized .github hook repo at v1.4.3.

Confidence Score: 5/5

Safe to merge — changes are limited to CI workflow configs and a pre-commit config; no source code is touched.

All three files are CI/tooling-only changes with no effect on the compiled library. The dual-event guard in conventional-pr.yml is correctly structured. The pre-commit workflow's null-delimited xargs pipeline correctly handles deleted files and spaces in paths. The .pre-commit-config.yaml changes are additive and non-breaking for existing contributors. The previously flagged fork-token issue is fully addressed by the pull_request_target routing.

No files require special attention.

Important Files Changed

Filename Overview
.github/workflows/conventional-pr.yml New caller stub; correctly guards dual-event trigger so same-repo PRs use pull_request and fork PRs use pull_request_target, granting a writable token only to the labeling path that never checks out code.
.github/workflows/pre-commit.yml Reworks pre-commit CI to use null-delimited xargs with --diff-filter=d and a dedicated cache step; handles deleted files and paths with spaces correctly.
.pre-commit-config.yaml Adds default_install_hook_types, conventional-pre-commit hook for commit-msg stage, fixes flake8 arg quoting, and moves add-license to the centralized org .github repo at v1.4.3.

Sequence Diagram

sequenceDiagram
    participant Dev as Developer
    participant GH as GitHub PR Event
    participant ConvPR as conventional-pr.yml
    participant OrgWF as org .github reusable workflow (v1.4.3)
    participant PreCommit as pre-commit.yml
    participant Cache as actions/cache

    Dev->>GH: Open / Edit / Sync PR

    GH->>ConvPR: pull_request (same-repo) OR pull_request_target (fork)
    Note over ConvPR: if-guard routes event type<br/>same-repo → pull_request<br/>fork → pull_request_target
    ConvPR->>OrgWF: "uses: conventional-pr.yml@v1.4.3<br/>permissions: pull-requests:write, issues:write"
    OrgWF-->>GH: Validate PR title, apply type labels, detect cherry-picks

    GH->>PreCommit: pull_request (all PRs)
    PreCommit->>Cache: "Restore ~/.cache/pre-commit<br/>key: OS + hash(.pre-commit-config.yaml)"
    PreCommit->>PreCommit: "git diff -z --diff-filter=d HEAD^1 HEAD"
    PreCommit->>PreCommit: xargs -0 --no-run-if-empty pre-commit run --files
    PreCommit-->>GH: Pass/Fail on modified files only

    Note over Dev: Local commit
    Dev->>Dev: pre-commit stage: isort, black, flake8, clang-format, codespell, add-license
    Dev->>Dev: commit-msg stage: conventional-pre-commit validates message format
Loading

Reviews (2): Last reviewed commit: "ci: harden CI workflows and configs per ..." | Re-trigger Greptile

Comment thread .github/workflows/conventional-pr.yml
Comment thread .github/workflows/conventional-pr.yml Outdated
permissions:
pull-requests: write
issues: write
uses: triton-inference-server/.github/.github/workflows/conventional-pr.yml@v1.4.2

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 PR description version mismatch

The PR description and its body text reference v1.4.1 throughout ("pinned tags current: v1.4.1"), but the actual pin here — and in .pre-commit-config.yaml line 87 — is v1.4.2. This is a documentation inconsistency; if v1.4.2 is the intended version (and CI is green), the description should be updated to avoid confusion when tracking which org-wide tag is live in this repo.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed - documentation drift: the descriptions have been refreshed to the current pinned tag v1.4.3 (they lagged behind the tag revisions; the tag exists and CI is green fleet-wide).

- conventional-pr stub: dual pull_request/pull_request_target triggers
  so fork PRs from external contributors get labeled too (the reusable
  workflow never checks out PR code); explicit contents:read; pinned
  v1.4.3.
- pre-commit workflow: robust modified-files runner (null-delimited
  paths, deletion-only PRs handled, deleted paths filtered, no
  undocumented -r flag, cache keyed on config hash).
- flake8 args quoted correctly (the flow-scalar form split at commas
  and silently reduced the select list).
- hooks pinned to .github v1.4.3.

TRI-1100
@yinggeh

yinggeh commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Commit-msg hook vs PR titles → developer_tools#61 thread: titles are CI-gated by the org-wide conventional-pr workflow; the commit-msg hook is local fast feedback and feeds multi-type labels. Happy to drop it if you prefer title-only.

Restrict to title-only makes more sense since not every commit has a type (most commits are trivial) and all commits likely share the same type.

@mc-nv

mc-nv commented Jul 22, 2026

Copy link
Copy Markdown
Contributor Author

Closing: the team is moving away from centralized org-level configuration in favor of per-repository self-contained setups (see triton-inference-server/server#8897 for the first decentralized implementation). Branch retained for reference. TRI-1100

@mc-nv mc-nv closed this Jul 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

chore Maintenance work, no production code change (chore: PRs) CI/CD Continuous integration and workflow changes (ci: PRs)

Development

Successfully merging this pull request may close these issues.

3 participants