Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 43 additions & 12 deletions .github/workflows/pr-unit.yml
Original file line number Diff line number Diff line change
Expand Up @@ -12,24 +12,54 @@ on:
- reopened

jobs:
determine-affected:
name: "Determine Affected Packages"
if: ${{ !contains(github.event.pull_request.labels.*.name, 'ci/skip-unit') }}
runs-on: ubuntu-latest
outputs:
packages: ${{ steps.affected.outputs.packages }}
has_packages: ${{ steps.affected.outputs.has_packages }}
steps:
- uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2
with:
fetch-depth: 0
- name: Setup Node.js
uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 # v4.4.0
with:
node-version: "18"
- name: Install dependencies
run: yarn install --frozen-lockfile --prefer-offline
- name: Determine affected packages
id: affected
run: |
if [[ "${{ contains(github.event.pull_request.labels.*.name, 'ci/run-all-unit') }}" == "true" ]]; then
echo "Label ci/run-all-unit detected, running all packages"
PACKAGES='["cdktn","cdktn-cli","@cdktn/hcl2cdk","@cdktn/hcl2json","@cdktn/provider-schema","@cdktn/provider-generator","@cdktn/commons","@cdktn/cli-core"]'
else
if [[ "${{ github.event_name }}" == "pull_request" ]]; then
BASE_SHA="${{ github.event.pull_request.base.sha }}"
else
BASE_SHA="${{ github.event.merge_group.base_sha }}"
fi
PACKAGES=$(bash tools/affected-packages.sh "$BASE_SHA")
fi
echo "packages=$PACKAGES" >> "$GITHUB_OUTPUT"
if [[ "$PACKAGES" == "[]" ]]; then
echo "has_packages=false" >> "$GITHUB_OUTPUT"
else
echo "has_packages=true" >> "$GITHUB_OUTPUT"
fi
echo "Affected packages: $PACKAGES"

all_unit_tests:
name: "All Unit Tests"
if: ${{ !contains(github.event.pull_request.labels.*.name, 'ci/skip-unit') }}
needs: determine-affected
if: ${{ needs.determine-affected.outputs.has_packages == 'true' }}
uses: ./.github/workflows/unit.yml
strategy:
fail-fast: false
matrix:
package:
[
cdktn,
cdktn-cli,
"@cdktn/hcl2cdk",
"@cdktn/hcl2json",
"@cdktn/provider-schema",
"@cdktn/provider-generator",
"@cdktn/commons",
"@cdktn/cli-core",
]
package: ${{ fromJSON(needs.determine-affected.outputs.packages) }}
terraform_version: ["1.6.5", "1.5.5"]
with:
concurrency_group_prefix: pr-all
Expand Down Expand Up @@ -141,6 +171,7 @@ jobs:
name: Unit Tests - PR - Result
needs:
[
determine-affected,
all_unit_tests,
cdktn,
cdktn_cli,
Expand Down
90 changes: 90 additions & 0 deletions RFCs/CI-IMPROVEMENTS.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
# Plan: Affected-Only Unit Tests in CI

## Context

Every PR currently runs **16 unit test jobs** (8 packages × 2 Terraform versions) regardless of what changed. A PR that only touches `CHANGELOG.md` or `.github/` still triggers all 250+ tests. This wastes CI minutes and slows down reviews.

The repo already has **Nx 20.8.2** installed with a project graph that correctly resolves workspace dependencies. `nx show projects --affected` works out of the box — verified locally that it correctly identifies `@cdktn/cli-core` + `cdktn-cli` + downstream examples when only `cli-core` was changed, and returns empty for non-package file changes.

## Approach: Nx `affected` with dynamic matrix

Use `nx show projects --affected` in a pre-job to build a dynamic matrix for `pr-unit.yml`. No new dependencies needed.

**Why Nx over alternatives:**

- Already installed and working (v20.8.2)
- Walks the dependency graph transitively (Lerna `--since` does not)
- No migration needed (Turborepo would require replacing Lerna)

## Files to Modify

1. **`.github/workflows/pr-unit.yml`** — add `determine-affected` job, make `all_unit_tests` matrix dynamic
2. **`tools/affected-packages.sh`** (new) — shell script to compute affected testable packages with fallback

## Implementation

### Step 1: Create `tools/affected-packages.sh`

Script that:

- Runs `npx nx show projects --affected --base=$BASE_REF --head=HEAD`
- Filters output to only the 8 testable packages (excludes examples, hcl-tools, etc.)
- Outputs a JSON array for GitHub Actions matrix consumption
- **Fallback**: if Nx fails or errors, outputs all 8 packages (safe default)
- **Empty case**: if no packages affected, outputs `[]`

### Step 2: Modify `pr-unit.yml`

Add a new `determine-affected` job before `all_unit_tests`:

```
determine-affected (ubuntu-latest, ~30s)
├─ checkout with fetch-depth: 0
├─ setup node + yarn install
├─ if label 'ci/run-all-unit' → output all 8 packages
├─ else → run tools/affected-packages.sh with PR base SHA
└─ outputs: packages (JSON array), has_packages (bool)
```

Modify `all_unit_tests`:

- Add `needs: determine-affected`
- Add condition: `has_packages == 'true'`
- Replace hardcoded `package` matrix with `fromJSON(needs.determine-affected.outputs.packages)`
- Keep `terraform_version: ["1.6.5", "1.5.5"]` hardcoded

Handle `merge_group` vs `pull_request`:

- PR: `base_sha = github.event.pull_request.base.sha`
- merge_group: `base_sha = github.event.merge_group.base_sha`

Add `determine-affected` to the `results` job `needs` list.

### Step 3: Add `ci/run-all-unit` label override

In the `determine-affected` job, check for label and short-circuit to all packages. This preserves manual control for cases where you want to force full test coverage.

## What stays unchanged

- **`unit.yml`** (reusable workflow) — no changes
- **All per-package label-based jobs** (`cdktn`, `cdktn_cli`, etc.) — kept as-is
- **Integration/provider/example workflows** — out of scope for this change (can add path filtering later)
- **Build caching, concurrency groups** — unchanged

## Expected Impact

| PR touches | Jobs before | Jobs after |
| ------------------------------- | ----------- | ------------------------ |
| Only docs/CI/changelog | 16 | 0 |
| Single leaf (`cdktn-cli`) | 16 | 2 (1 pkg × 2 TF) |
| `@cdktn/hcl2json` | 16 | 10 (5 pkgs × 2 TF) |
| Core `cdktn` lib | 16 | 16 (all affected) |
| Root `package.json`/`yarn.lock` | 16 | 16 (Nx treats as global) |

## Verification

1. Run `bash tools/affected-packages.sh origin/main` locally on various branches to verify output
2. Open a docs-only PR → confirm 0 unit test jobs run
3. Open a PR touching `@cdktn/commons` → confirm only `commons` + dependents are tested
4. Apply `ci/run-all-unit` label → confirm all 16 jobs run
5. Verify `results` job still correctly reports pass/fail
65 changes: 65 additions & 0 deletions tools/affected-packages.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
#!/usr/bin/env bash
# Copyright (c) HashiCorp, Inc.
# SPDX-License-Identifier: MPL-2.0

# Determine which testable packages are affected by changes between BASE and HEAD.
# Outputs a JSON array suitable for GitHub Actions matrix consumption.
#
# Usage:
# bash tools/affected-packages.sh <base_ref>
# bash tools/affected-packages.sh origin/main
#
# Output examples:
# ["cdktn","cdktn-cli","@cdktn/commons"] # some packages affected
# [] # nothing affected

set -euo pipefail

BASE_REF="${1:?Usage: affected-packages.sh <base_ref>}"

# The 8 packages that have unit tests
TESTABLE_PACKAGES=(
"cdktn"
"cdktn-cli"
"@cdktn/hcl2cdk"
"@cdktn/hcl2json"
"@cdktn/provider-schema"
"@cdktn/provider-generator"
"@cdktn/commons"
"@cdktn/cli-core"
)

all_packages_json() {
printf '%s\n' "${TESTABLE_PACKAGES[@]}" | jq -Rsc 'split("\n") | map(select(length > 0))'
}

# Fallback: return all packages on any error
fallback() {
echo "::warning::Nx affected detection failed, falling back to all packages" >&2
all_packages_json
exit 0
}
trap fallback ERR

# Get affected projects from Nx
AFFECTED=$(npx nx show projects --affected --base="$BASE_REF" --head=HEAD 2>/dev/null)

if [[ -z "$AFFECTED" ]]; then
# No projects affected at all
echo "[]"
exit 0
fi

# Filter to only testable packages
MATCHED=()
for pkg in "${TESTABLE_PACKAGES[@]}"; do
if echo "$AFFECTED" | grep -qx "$pkg"; then
MATCHED+=("$pkg")
fi
done

if [[ ${#MATCHED[@]} -eq 0 ]]; then
echo "[]"
else
printf '%s\n' "${MATCHED[@]}" | jq -Rsc 'split("\n") | map(select(length > 0))'
fi
Loading