Skip to content

ci: allow nvskills signing account for dco#731

Merged
johnnygreco merged 1 commit into
mainfrom
johnny/ci-allow-nvskills-dco
Jun 2, 2026
Merged

ci: allow nvskills signing account for dco#731
johnnygreco merged 1 commit into
mainfrom
johnny/ci-allow-nvskills-dco

Conversation

@johnnygreco
Copy link
Copy Markdown
Contributor

📋 Summary

Allow the NVSkills signing service account to pass the DCO Assistant allowlist. This lets NVSkills-generated signature commits, such as the one currently on #718, satisfy DCO without requiring the service account to manually sign.

🔗 Related Issue

N/A

🔄 Changes

  • Add svc-nvskills-signing to the DCO Assistant allowlist in .github/workflows/dco-assistant.yml.

🧪 Testing

  • git diff --check
  • Unit tests added/updated: N/A — CI workflow allowlist only
  • E2E tests added/updated: N/A — CI workflow allowlist only

✅ Checklist

  • Follows commit message conventions
  • Commits are signed off (DCO)
  • Architecture docs updated: N/A — CI workflow change only

Signed-off-by: Johnny Greco <jogreco@nvidia.com>
@johnnygreco johnnygreco requested a review from a team as a code owner June 2, 2026 17:08
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Jun 2, 2026

Greptile Summary

This PR adds svc-nvskills-signing to the DCO Assistant allowlist in the CI workflow, allowing NVSkills service-account commits to pass the DCO check without manual signing. The change is a single-line config update with no logic changes.

  • Appends svc-nvskills-signing to the allowlist field in .github/workflows/dco-assistant.yml, mirroring the existing dependabot[bot] exemption pattern.

Confidence Score: 5/5

Minimal-risk CI config change — appends one service account name to an existing allowlist with no effect on code logic or secrets.

The change is a single-line edit to a comma-separated allowlist value. It follows the same pattern already used for dependabot[bot] and touches nothing else in the workflow.

No files require special attention.

Important Files Changed

Filename Overview
.github/workflows/dco-assistant.yml Adds svc-nvskills-signing to the DCO Assistant allowlist alongside the existing dependabot[bot] entry, allowing NVSkills-generated signature commits to bypass the DCO signing requirement.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[PR opened / synchronized] --> B{Trusted Agentic CI?}
    B -- yes --> Z[Skip DCO check]
    B -- no --> C[DCO Assistant action runs]
    C --> D{Commit author in allowlist?}
    D -- yes
dependabot[bot]
svc-nvskills-signing --> E[DCO check passes]
    D -- no --> F{Has signed DCO?}
    F -- yes --> E
    F -- no --> G[Post signing prompt on PR]
Loading

Reviews (1): Last reviewed commit: "ci: allow nvskills signing account for d..." | Re-trigger Greptile

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 2, 2026

Code Review: PR #731ci: allow nvskills signing account for dco

Summary

Single-line change to .github/workflows/dco-assistant.yml that adds the svc-nvskills-signing service account to the DCO Assistant allowlist. This lets NVSkills-generated signature commits (e.g., on PR #718) bypass the DCO sign-off requirement without the service account having to manually sign the DCO. No application code or tests are touched.

Findings

Correctness

  • The diff changes allowlist: dependabot[bot] to allowlist: dependabot[bot],svc-nvskills-signing. Comma-separated values are the documented input format for cla-assistant/github-action, and there is no whitespace around the comma — matches the upstream parser.
  • The new entry is a GitHub username (no [bot] suffix). For a regular service account this is correct; only GitHub Apps require the [bot] suffix. Worth a quick sanity check that svc-nvskills-signing is in fact a user account, not a GitHub App, otherwise the allowlist will not match.
  • No other occurrences of the allowlist value exist in the repo, so there is no second source of truth to keep in sync.

Project conventions

  • File header / SPDX licensing is unchanged on a workflow file, consistent with the rest of .github/workflows/.
  • Commit and PR titles follow the repo's conventional-commit style (ci: …).
  • The change does not touch the data_designer packages, so the import-direction, lazy-import, and typing invariants from AGENTS.md are not in play here.

Security

  • Adding an account to the DCO allowlist is a trust decision: any commit authored by svc-nvskills-signing will be treated as DCO-signed without an explicit Signed-off-by line. This is appropriate for an internal NVIDIA signing service that already attests provenance, but it is worth confirming that the service account is access-controlled and cannot be impersonated by external contributors (e.g., its commits only appear via a controlled bot pipeline, not via arbitrary PR pushes).
  • No secrets, tokens, or permissions blocks are modified. The workflow's permissions: scope is unchanged.

Tests / coverage

  • N/A — workflow allowlist entry. There is no practical unit-test surface; verification will be the next NVSkills-signed PR passing the DCO check. The PR description acknowledges this.

Suggestions (minor, non-blocking)

  • Consider adding an inline YAML comment next to the allowlist explaining why svc-nvskills-signing is present (e.g., # NVSkills signing service — commits are pre-attested). The next person to touch this file will otherwise have to dig through git history to learn why a bare username sits next to dependabot[bot].
  • Optional: alphabetize the allowlist (dependabot[bot],svc-nvskills-signing happens to already be alphabetical, so no action needed today, but worth establishing as a convention if more entries land later).

Verdict

Looks good. Low-risk, single-line CI change with a clear rationale. Approve once someone with knowledge of the NVSkills signing pipeline confirms (a) the account name is correct as written (no [bot] suffix needed) and (b) the account is sufficiently access-controlled that allowlisting it is safe.

@johnnygreco johnnygreco merged commit 7bd5381 into main Jun 2, 2026
63 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants