|
| 1 | +--- |
| 2 | +name: code-review |
| 3 | +description: >- |
| 4 | + Review a pull request or code change in tektoncd/catalog. Use when asked to |
| 5 | + review a PR, evaluate a diff, or assess code quality. Applies Tekton community |
| 6 | + standards, TEP-0003 catalog organization, task authoring recommendations, and |
| 7 | + catalog-specific conventions (directory structure, metadata, tests). |
| 8 | +license: Apache-2.0 |
| 9 | +metadata: |
| 10 | + project: tekton-catalog |
| 11 | +allowed-tools: Read Grep Glob Bash(git diff:*) Bash(git log:*) Bash(yamllint:*) |
| 12 | +--- |
| 13 | + |
| 14 | +# Code Review |
| 15 | + |
| 16 | +tektoncd/catalog follows [Tekton community review standards](https://github.com/tektoncd/community/blob/main/standards.md) |
| 17 | +and [TEP-0003 Catalog Organization](https://github.com/tektoncd/community/blob/master/teps/0003-tekton-catalog-organization.md). |
| 18 | + |
| 19 | +## Review Checklist |
| 20 | + |
| 21 | +### Catalog Organization (TEP-0003) |
| 22 | + |
| 23 | +- [ ] File path follows `<kind>/<name>/<version>/name.yaml` (kind = task, stepaction, or pipeline) |
| 24 | +- [ ] Resource YAML file name matches the resource name |
| 25 | +- [ ] `README.md` exists at `<kind>/<name>/<version>/README.md` |
| 26 | +- [ ] `OWNERS` file exists at `<kind>/<name>/OWNERS` (or version-level) |
| 27 | +- [ ] Version directory name is a valid semver-like format (e.g. `0.1`, `0.2`) |
| 28 | + |
| 29 | +### Mandatory Metadata |
| 30 | + |
| 31 | +- [ ] Label `app.kubernetes.io/version` matches the directory version |
| 32 | +- [ ] Annotation `tekton.dev/pipelines.minVersion` is present and accurate |
| 33 | +- [ ] Annotation `tekton.dev/categories` is present (e.g. Git, Build, Deploy, CLI) |
| 34 | +- [ ] Annotation `tekton.dev/tags` is present with comma-separated tags |
| 35 | +- [ ] Annotation `tekton.dev/displayName` is present (optional but recommended) |
| 36 | +- [ ] Annotation `tekton.dev/platforms` lists supported platforms (optional) |
| 37 | +- [ ] `spec.description` follows the convention: one-line summary followed by |
| 38 | + detailed paragraph(s) |
| 39 | + |
| 40 | +### Task Authoring Quality |
| 41 | + |
| 42 | +- [ ] **No `$(params.*)` interpolation inside `script` blocks** — this is a |
| 43 | + security vulnerability (code injection) and reliability issue. Parameters |
| 44 | + must be passed via `env` or `args` and referenced as `$ENV_VAR` or `$1`. |
| 45 | + See [recommendations.md](../../../recommendations.md) |
| 46 | +- [ ] Images are referenced by digest (`image@sha256:...`) where possible, |
| 47 | + not by mutable tags (`:latest`) |
| 48 | +- [ ] Steps run as non-root and non-privileged unless explicitly required. |
| 49 | + If root is needed, use `securityContext.runAsUser: 0` explicitly |
| 50 | +- [ ] Parameters have default values where reasonable |
| 51 | +- [ ] Workspaces have clear descriptions. Tasks should use **at most one |
| 52 | + writeable workspace** (Tekton recommendation) |
| 53 | +- [ ] Script steps use `#!/usr/bin/env bash` (or appropriate shebang) |
| 54 | + with `set -e` for error handling |
| 55 | +- [ ] No hardcoded values that should be parameters |
| 56 | +- [ ] Results are documented and used only for small data (commit SHAs, |
| 57 | + branch names). Large data should use a Workspace instead |
| 58 | +- [ ] Tasks are idempotent — safe to re-execute |
| 59 | +- [ ] `tekton.dev/pipelines.minVersion` is set to the lowest version that |
| 60 | + supports the features used (portability across Pipeline versions) |
| 61 | + |
| 62 | +### YAML Quality |
| 63 | + |
| 64 | +- [ ] `yamllint` passes with the project's `.yamllint` config |
| 65 | +- [ ] No trailing whitespace |
| 66 | +- [ ] Consistent indentation (2 spaces) |
| 67 | +- [ ] Proper quoting of strings where needed |
| 68 | + |
| 69 | +### Testing |
| 70 | + |
| 71 | +- [ ] New tasks or changed functionality include tests in `tests/` directory |
| 72 | +- [ ] Test directory contains `run.yaml` (TaskRun or PipelineRun) |
| 73 | +- [ ] Pre-apply hook scripts (`pre-apply-task-hook.sh`, |
| 74 | + `pre-apply-taskrun-hook.sh`) are used where setup is needed |
| 75 | +- [ ] For tasks calling external APIs, fixtures are provided in |
| 76 | + `tests/fixtures/` for go-rest-api-test |
| 77 | +- [ ] Tests do not depend on external services without fixture mocking |
| 78 | + |
| 79 | +### New Version Workflow |
| 80 | + |
| 81 | +- [ ] If bumping a version (e.g. 0.1 to 0.2), the old version was |
| 82 | + copied first in a separate commit, then modified in a subsequent commit |
| 83 | +- [ ] Changes from the previous version are clearly visible in the diff |
| 84 | + (not buried in a full copy) |
| 85 | + |
| 86 | +### Documentation |
| 87 | + |
| 88 | +- [ ] `README.md` documents all parameters with types and defaults |
| 89 | +- [ ] `README.md` documents all workspaces |
| 90 | +- [ ] `README.md` documents all results (if any) |
| 91 | +- [ ] `README.md` includes usage example(s) |
| 92 | +- [ ] `samples/` directory contains example TaskRun/PipelineRun YAMLs |
| 93 | + |
| 94 | +### Security |
| 95 | + |
| 96 | +- [ ] **No `$(params.*)` interpolation in scripts** — critical injection risk |
| 97 | + (repeat check — this is the most common security issue in catalog tasks) |
| 98 | +- [ ] No secrets or credentials hardcoded in YAML |
| 99 | +- [ ] Sensitive data passed via Kubernetes Secrets or workspaces |
| 100 | +- [ ] Container images come from trusted registries |
| 101 | +- [ ] No use of `privileged: true` without justification |
| 102 | + |
| 103 | +## What to Approve |
| 104 | + |
| 105 | +Approve when: |
| 106 | +- All checklist items pass |
| 107 | +- Tests cover the changed behavior |
| 108 | +- The change is focused (one concern per PR) |
| 109 | +- Commit messages follow [Tekton commit conventions](https://github.com/tektoncd/community/blob/main/standards.md#commit-messages) |
| 110 | +- New version workflow is followed (copy + modify in separate commits) |
| 111 | + |
| 112 | +## What to Block |
| 113 | + |
| 114 | +Block (request changes) when: |
| 115 | +- `$(params.*)` is used inside `script` blocks (injection vulnerability) |
| 116 | +- TEP-0003 structure is not followed |
| 117 | +- Mandatory metadata labels/annotations are missing |
| 118 | +- Images use mutable tags without justification |
| 119 | +- Tests are missing for new or changed tasks |
| 120 | +- Security issues (hardcoded secrets, unnecessary privileges) |
| 121 | +- `yamllint` fails |
| 122 | +- Version copy and modifications are squashed into a single commit |
0 commit comments