diff --git a/README.md b/README.md index 18d2a56..8c7e95c 100644 --- a/README.md +++ b/README.md @@ -61,7 +61,7 @@ Track work across the four RHDH Jira projects. ### PR Review -- **[rhdh-pr-review](./skills/rhdh-pr-review/SKILL.md)** — Test PR changes on a live RHDH cluster. Swaps CI images, verifies code changes, and reports findings. +- **[rhdh-pr-review](./skills/rhdh-pr-review/SKILL.md)** — Test PR changes on a live RHDH cluster. Supports rhdh-operator PRs (deploys full OLM bundle or manifests) and rhdh-chart PRs (deploys chart from PR branch via Helm). Verifies code changes and reports findings. ### Test Plan diff --git a/skills/rhdh-pr-review/SKILL.md b/skills/rhdh-pr-review/SKILL.md index 5d9a48d..8ef78aa 100644 --- a/skills/rhdh-pr-review/SKILL.md +++ b/skills/rhdh-pr-review/SKILL.md @@ -1,6 +1,6 @@ --- name: rhdh-pr-review -description: Test PR changes on a live RHDH cluster. Fetches CI-built images from PR comments, checks cluster status (deploying if needed), deploys the full PR operator bundle or manifests (not just image swap), actively verifies the code changes by exercising affected code paths on the cluster, and closes with findings including best-practice and security assessment. Use when asked to review an rhdh-operator PR, test PR changes on a cluster, deploy PR images for testing, or deploy PR bundle. Also use when user mentions "operator PR", "review PR", or "test this PR on my cluster". Currently supports rhdh-operator PRs. +description: Test PR changes on a live RHDH cluster. Fetches CI-built images or checks out chart branches, checks cluster status (deploying if needed), deploys the full PR operator bundle/manifests or Helm chart, actively verifies the code changes by exercising affected code paths on the cluster, and closes with findings including best-practice and security assessment. Use when asked to review an rhdh-operator PR, rhdh-chart PR, test PR changes on a cluster, deploy PR images for testing, or deploy PR bundle. Also use when user mentions "operator PR", "chart PR", "helm chart PR", "review PR", or "test this PR on my cluster". --- @@ -34,6 +34,13 @@ For non-OLM installs, fetch and apply the PR branch's `install.yaml` (CRDs, RBAC Both paths preserve existing Backstage CRs, config, and Keycloak state — only the operator-side resources are replaced. + +Deploy the full chart from the PR branch, not just a values override. +PR changes to templates, helpers, schema, or dependencies are baked into the chart itself — a values-only change would miss them. +Use `helm upgrade` pointing at the local chart directory from the PR branch with `--reuse-values` to preserve existing user configuration. +See `references/chart-pr-testing.md` for deployment commands. + + @@ -45,6 +52,7 @@ Both paths preserve existing Backstage CRs, config, and Keycloak state — only *For testing PR changes on a live RHDH cluster* 1. **Review rhdh-operator PR** — Deploy PR operator bundle on cluster and get review checklist +2. **Review rhdh-chart PR** — Deploy PR chart on cluster and get review checklist **Wait for response before proceeding.** @@ -56,9 +64,10 @@ Both paths preserve existing Backstage CRs, config, and Keycloak state — only | Response | Workflow | |----------|----------| -| 1, "operator", "rhdh-operator", a PR number, "review" | Route to `workflows/review-operator-pr.md` | +| 1, "operator", "rhdh-operator", a PR number + operator context | Route to `workflows/review-operator-pr.md` | +| 2, "chart", "rhdh-chart", "helm", a PR number + chart context | Route to `workflows/review-chart-pr.md` | -**To route:** Read `workflows/review-operator-pr.md` and follow its process. +**To route:** Read the corresponding workflow file and follow its process. @@ -66,7 +75,11 @@ Both paths preserve existing Backstage CRs, config, and Keycloak state — only | Reference | Purpose | Path | |-----------|---------|------| -| operator-pr-images | CI image extraction and validation | `references/operator-pr-images.md` | +| shared-pr-fetch | PR context fetching (shared) | `references/shared-pr-fetch.md` | +| shared-verification | Active verification process (shared) | `references/shared-verification.md` | +| shared-findings-structure | Findings report structure (shared) | `references/shared-findings-structure.md` | +| operator-pr-images | CI image extraction and validation (operator) | `references/operator-pr-images.md` | +| chart-pr-testing | Chart CI behavior, local validation, deployment (chart) | `references/chart-pr-testing.md` | | github-reference | gh CLI patterns, PR queries | `../rhdh/references/github-reference.md` (if unavailable, use standard `gh` CLI patterns) | | rhdh-repos | RHDH ecosystem repository map | `../rhdh/references/rhdh-repos.md` (if unavailable, see CONTEXT.md for repo list) | @@ -82,6 +95,8 @@ Both paths preserve existing Backstage CRs, config, and Keycloak state — only -See `workflows/review-operator-pr.md` `` for the full checklist. +See the corresponding workflow's `` for the full checklist: +- Operator PRs: `workflows/review-operator-pr.md` +- Chart PRs: `workflows/review-chart-pr.md` diff --git a/skills/rhdh-pr-review/references/chart-pr-testing.md b/skills/rhdh-pr-review/references/chart-pr-testing.md new file mode 100644 index 0000000..e964641 --- /dev/null +++ b/skills/rhdh-pr-review/references/chart-pr-testing.md @@ -0,0 +1,92 @@ +# Reference: rhdh-chart PR Testing + +How to validate and deploy chart changes from a PR. The CI workflow definition lives in the rhdh-chart repo at `.github/workflows/test.yaml` and `.github/actions/test-charts/action.yml` — read them for the authoritative CI behavior. + + + +## Key Facts + +- Chart CI does NOT build container images — it runs `ct lint` + `ct install` on a KIND cluster +- Charts are published to `oci://quay.io/rhdh/chart` only on merge (via `chart-releaser-action`) +- To test a PR's chart changes, you must check out the PR branch and deploy from the local checkout +- Pre-commit hooks auto-generate `README.md` (helm-docs), `values.schema.json` (jsonschema-dereference), and update Helm dependencies +- Chart architecture details are in `../../rhdh/references/rhdh-repos.md` under the `rhdh-chart` section — do not re-document here + + + + + +## Getting the PR Chart Locally + +```bash +REPO="redhat-developer/rhdh-chart" +PR_NUMBER= + +# Clone and checkout the PR branch +gh pr checkout $PR_NUMBER --repo $REPO --detach +cd rhdh-chart + +# Update Helm dependencies (required before template/install) +helm repo add bitnami https://charts.bitnami.com/bitnami +helm dependency update charts/backstage +``` + +If you already have a local clone: + +```bash +PR_BRANCH=$(gh pr view $PR_NUMBER --repo $REPO --json headRefName --jq '.headRefName') +git fetch origin $PR_BRANCH +git checkout FETCH_HEAD +helm dependency update charts/backstage +``` + + + + + +## Local Validation + +```bash +# Lint the chart +ct lint --config ct-lint.yaml + +# Render templates to verify they produce valid YAML +helm template rhdh charts/backstage/ --debug + +# Run pre-commit hooks (schema regen, helm-docs, dependency update) +pre-commit run --all-files +``` + +**Check Chart.yaml version was bumped** — the PR checklist requires it: + +```bash +# Compare with base branch +git diff origin/main -- charts/backstage/Chart.yaml | grep '^[+-]version:' +``` + + + + + +## Deploying to a Cluster + +**OpenShift:** + +```bash +CLUSTER_ROUTER_BASE=$(oc get route console -n openshift-console -o jsonpath='{.status.ingress[0].host}' | sed 's/^console-openshift-console\.//') + +helm upgrade -i redhat-developer-hub charts/backstage/ \ + --set global.clusterRouterBase=$CLUSTER_ROUTER_BASE \ + --set route.enabled=true +``` + +**Vanilla Kubernetes:** + +```bash +helm upgrade -i redhat-developer-hub charts/backstage/ \ + --set route.enabled=false \ + --set upstream.ingress.enabled=true \ + --set global.host=rhdh.127.0.0.1.sslip.io +``` + + diff --git a/skills/rhdh-pr-review/references/shared-findings-structure.md b/skills/rhdh-pr-review/references/shared-findings-structure.md new file mode 100644 index 0000000..040d418 --- /dev/null +++ b/skills/rhdh-pr-review/references/shared-findings-structure.md @@ -0,0 +1,49 @@ +# Reference: Findings & Recommendations Structure (Shared) + +Shared Phase 7 for all PR review workflows. The calling workflow defines repo-specific best-practice bullets and rollback commands before referencing this file. + + + +## Findings & Recommendations + +Synthesize the verification results and provide a complete review assessment. + +### 7.1 Verification summary + +Summarize what was tested and the results: + +| Category | Test performed | Result | Evidence | +|---|---|---|---| +| *[category]* | *[what was tested]* | Pass/Fail | *[key observation]* | + +### 7.2 Best practice assessment + +Review the PR's approach against the repo's development best practices. Reference `../../rhdh/references/rhdh-repos.md` for conventions. Use the repo-specific best-practice bullets defined by the calling workflow. + +### 7.3 Security review + +Evaluate the changes from a security perspective: + +- Are secrets handled safely (no plaintext logging, proper Secret resources)? +- Do RBAC changes follow least-privilege principle? +- Are container image references pinned appropriately? +- Are new network exposures (ports, routes, service accounts) intentional and documented? +- Do dependency updates introduce known CVEs? +- Are user-supplied inputs validated before use in resource names or labels? + +Add any repo-specific security concerns defined by the calling workflow. + +### 7.4 Improvement suggestions + +Based on the findings, suggest concrete improvements if any: + +- Code or template changes needed (reference specific files and lines from the diff) +- Missing test coverage for the changed code paths +- Documentation gaps +- Configuration or operational concerns + +### 7.5 Rollback instructions + +Present the rollback commands recorded in Phase 4. Include verification that the rollback succeeded. + + diff --git a/skills/rhdh-pr-review/references/shared-pr-fetch.md b/skills/rhdh-pr-review/references/shared-pr-fetch.md new file mode 100644 index 0000000..319a73a --- /dev/null +++ b/skills/rhdh-pr-review/references/shared-pr-fetch.md @@ -0,0 +1,30 @@ +# Reference: Fetch PR Context (Shared) + +Shared Phase 1 for all PR review workflows. The calling workflow must set `REPO` and `PR_NUMBER` before referencing this file. + + + +## Fetch PR Context + +```bash +gh pr view $PR_NUMBER --repo $REPO \ + --json number,title,state,author,body,files,createdAt,headRefOid,baseRefName +``` + +Validate: +- PR state is `OPEN` (warn if merged or closed — artifacts may still work but PR is not active) +- PR belongs to the expected `$REPO` + +Fetch the diff for later checklist generation: + +```bash +gh pr diff $PR_NUMBER --repo $REPO +``` + +Save the changed file list for Phase 5: + +```bash +gh pr view $PR_NUMBER --repo $REPO --json files --jq '.files[].path' +``` + + diff --git a/skills/rhdh-pr-review/references/shared-verification.md b/skills/rhdh-pr-review/references/shared-verification.md new file mode 100644 index 0000000..d4450b6 --- /dev/null +++ b/skills/rhdh-pr-review/references/shared-verification.md @@ -0,0 +1,36 @@ +# Reference: Active Verification Process (Shared) + +Shared Phase 6 for all PR review workflows. This phase verifies the PR's specific changes on the cluster — not generic health checks. The goal is to exercise the exact code paths the PR modified and capture evidence that the behavioral change works as intended. + + + +## Active Verification + +### 6.1 Analyze the diff + +Read the diff hunks from Phase 1. For each changed file, understand: + +- What the code did **before** the change +- What it does **after** +- What behavioral difference this introduces on a running cluster + +Map each change to a concrete cluster-observable effect — something you can trigger and measure. If a change has no cluster-observable effect (e.g., pure refactor with identical behavior, documentation-only update, CI config change), state that explicitly and explain why. + +### 6.2 Propose a verification plan + +Present the plan to the user. For each test, specify: + +- **What to do**: the exact cluster action +- **What to observe**: where to look (logs, resource spec, status, events, HTTP response) +- **Pass criteria**: what output means the change works +- **Fail criteria**: what output means the change is broken + +**STOP. Do not run any verification commands. Present the plan and wait for the user to accept it before proceeding to 6.3.** + +### 6.3 Execute the plan + +Only after the user accepts the plan: + +Run each verification step on the cluster. For every step, capture the actual command output as evidence. Do not summarize — show the raw output so the user can see exactly what happened. + + diff --git a/skills/rhdh-pr-review/workflows/review-chart-pr.md b/skills/rhdh-pr-review/workflows/review-chart-pr.md new file mode 100644 index 0000000..28349f4 --- /dev/null +++ b/skills/rhdh-pr-review/workflows/review-chart-pr.md @@ -0,0 +1,444 @@ +# Workflow: Review rhdh-chart PR on Live Cluster + +Check out a PR's chart changes, validate locally, deploy the full chart to a running RHDH cluster, and generate a targeted review checklist from the diff. + + + +Read these reference files before starting: + +1. `../references/shared-pr-fetch.md` — Shared Phase 1 (PR context fetching) +2. `../references/shared-verification.md` — Shared Phase 6 (active verification) +3. `../references/shared-findings-structure.md` — Shared Phase 7 (findings structure) +4. `../references/chart-pr-testing.md` — Chart CI behavior, local validation, deployment commands +5. `../../rhdh/references/github-reference.md` — gh CLI patterns + + + + + +| Requirement | Details | +|-------------|---------| +| **Input** | PR number for rhdh-chart (or full PR URL) | +| **Access** | Read access to `redhat-developer/rhdh-chart` | +| **Tools** | `gh` CLI authenticated, `helm` CLI available, `oc` or `kubectl` available | +| **Cluster** | Running OpenShift or Kubernetes cluster (will offer to deploy if no RHDH instance) | + + + + + +## Phase 1: Fetch PR Context + +```bash +REPO="redhat-developer/rhdh-chart" +PR_NUMBER= +``` + +Read `../references/shared-pr-fetch.md` and follow the `` instructions using the `REPO` and `PR_NUMBER` variables above. + +--- + +## Phase 2: Local Chart Validation + +Chart CI does NOT build container images — it runs `ct lint` + `ct install` on KIND. To test on a real cluster, check out the PR branch and validate locally first. + +### 2.1 Check out the PR branch + +```bash +# If rhdh-chart is already cloned locally +CHART_DIR=$(find .. -maxdepth 1 -name "rhdh-chart" -type d 2>/dev/null | head -1) +if [ -z "$CHART_DIR" ]; then + gh repo clone $REPO /tmp/rhdh-chart-pr-$PR_NUMBER + CHART_DIR="/tmp/rhdh-chart-pr-$PR_NUMBER" +fi + +cd "$CHART_DIR" +PR_BRANCH=$(gh pr view $PR_NUMBER --repo $REPO --json headRefName --jq '.headRefName') +git fetch origin $PR_BRANCH +git checkout FETCH_HEAD +``` + +### 2.2 Update Helm dependencies + +```bash +helm repo add bitnami https://charts.bitnami.com/bitnami 2>/dev/null +helm dependency update charts/backstage +``` + +### 2.3 Validate chart rendering + +```bash +helm template rhdh charts/backstage/ --debug 2>&1 | head -50 +``` + +If `helm template` fails, the chart has rendering errors that must be fixed before deployment. + +### 2.4 Check Chart.yaml version bump + +The PR checklist requires a version bump for each changed chart: + +```bash +TARGET_BRANCH=$(gh pr view $PR_NUMBER --repo $REPO --json baseRefName --jq '.baseRefName') +git diff origin/$TARGET_BRANCH -- charts/backstage/Chart.yaml | grep '^[+-]version:' +``` + +If no version change is found, flag it — the PR checklist explicitly requires a bump per Semantic Versioning. + +### 2.5 Check pre-commit hooks (optional) + +If `pre-commit` is available: + +```bash +pre-commit run --all-files 2>&1 +``` + +This validates: +- `README.md` is regenerated (helm-docs) +- `values.schema.json` is regenerated (jsonschema-dereference) +- Helm dependencies are up to date + +--- + +## Phase 3: Ensure a Running RHDH Cluster + +### 3.1 Verify cluster access + +```bash +# Try OpenShift first, fall back to kubectl +oc whoami 2>&1 || kubectl cluster-info 2>/dev/null | head -2 +``` + +### 3.2 Check for existing RHDH Helm release + +```bash +helm list -A 2>/dev/null | grep -E 'backstage|rhdh|redhat-developer-hub' +``` + +### 3.3 Check for existing RHDH pods + +```bash +# kubectl works on both OCP and vanilla K8s +kubectl get pods -A --no-headers 2>/dev/null | grep -i backstage +``` + +### 3.4 Decision tree + +| Cluster state | Action | +|---------------|--------| +| Helm release exists + RHDH pods running | Skip to Phase 4 | +| Cluster accessible but no RHDH | Deploy RHDH via Helm on existing cluster (see 3.5) | +| No cluster access | Provision a cluster via rhdh-test-instance (see 3.5) | + +### 3.5 Provision or deploy RHDH + +Use `redhat-developer/rhdh-test-instance` — see `../../rhdh/references/rhdh-repos.md` for its capabilities, Makefile targets, and `/test deploy` slash commands. Read the repo's own README for full usage. + +- **No cluster at all** → use rhdh-test-instance PR workflow: comment `/test deploy helm 4h` on a PR. Use a 4h TTL (reviews are short-lived). Match the version to the PR's target branch. +- **Cluster accessible but no RHDH** → use rhdh-test-instance locally: `make deploy-helm VERSION=`. Read the repo's README for `.env` setup. + +Once RHDH pods are running, proceed to Phase 4. + +--- + +## Phase 4: Deploy PR Chart + +### 4.1 Detect existing install method + +```bash +# Check if current RHDH was deployed via Helm +HELM_RELEASE=$(helm list -A --no-headers 2>/dev/null | grep -E 'backstage|rhdh|redhat-developer-hub' | awk '{print $1}') +HELM_NS=$(helm list -A --no-headers 2>/dev/null | grep -E 'backstage|rhdh|redhat-developer-hub' | awk '{print $2}') + +# Check if deployed via operator instead +OLM_MANAGED=$(oc get subscription -A 2>/dev/null | grep -i rhdh) +``` + +- If Helm release found → proceed with `helm upgrade` (use 4.3) +- If OLM-managed (operator) → warn: "This cluster has RHDH deployed via operator. Use the `review-operator-pr` workflow instead, or install a separate Helm-based RHDH in a different namespace." + +### 4.2 Record current state (for rollback) + +```bash +HELM_REVISION=$(helm history $HELM_RELEASE -n $HELM_NS --max 1 --no-headers 2>/dev/null | awk '{print $1}') +echo "Current Helm release: $HELM_RELEASE in namespace $HELM_NS, revision $HELM_REVISION" + +# Save current values for reference +helm get values $HELM_RELEASE -n $HELM_NS -o yaml > /tmp/rollback-chart-values.yaml +echo "Saved current values to /tmp/rollback-chart-values.yaml" +``` + +### 4.3 Deploy PR chart from local checkout + +**IMPORTANT:** Deploy the full chart from the PR branch — not just a values override. PR changes to templates, helpers, schema, or dependencies are baked into the chart itself. A values-only change would miss them. + +**OpenShift:** + +```bash +CLUSTER_ROUTER_BASE=$(oc get route console -n openshift-console \ + -o jsonpath='{.status.ingress[0].host}' | sed 's/^console-openshift-console\.//') + +helm upgrade $HELM_RELEASE "$CHART_DIR/charts/backstage/" \ + -n $HELM_NS \ + --set global.clusterRouterBase=$CLUSTER_ROUTER_BASE \ + --set route.enabled=true \ + --reuse-values +``` + +**Vanilla Kubernetes:** + +```bash +helm upgrade $HELM_RELEASE "$CHART_DIR/charts/backstage/" \ + -n $HELM_NS \ + --set route.enabled=false \ + --set upstream.ingress.enabled=true \ + --set global.host=rhdh.127.0.0.1.sslip.io \ + --reuse-values +``` + +The `--reuse-values` flag preserves any existing user configuration (dynamic plugins, app-config, secrets) while applying the PR's chart changes (templates, defaults, schema). + +### 4.4 Wait for rollout + +```bash +# Wait for the RHDH deployment to roll out +RHDH_DEPLOY=$(kubectl get deployment -n $HELM_NS --no-headers \ + -o custom-columns=NAME:.metadata.name 2>/dev/null | grep -E 'backstage|rhdh') + +kubectl rollout status deployment/$RHDH_DEPLOY -n $HELM_NS --timeout=300s +``` + +### 4.5 Verify the deployment + +```bash +# Check pods are running +kubectl get pods -n $HELM_NS + +# Check RHDH is accessible +RHDH_URL="" +# OpenShift: check route +ROUTE_HOST=$(oc get route -n $HELM_NS --no-headers -o custom-columns=HOST:.spec.host 2>/dev/null | head -1) +if [ -n "$ROUTE_HOST" ]; then + RHDH_URL="https://$ROUTE_HOST" +fi + +# Kubernetes: check ingress +if [ -z "$RHDH_URL" ]; then + INGRESS_HOST=$(kubectl get ingress -n $HELM_NS --no-headers -o custom-columns=HOST:.spec.rules[0].host 2>/dev/null | head -1) + if [ -n "$INGRESS_HOST" ]; then + RHDH_URL="http://$INGRESS_HOST" + fi +fi + +if [ -n "$RHDH_URL" ]; then + echo "RHDH accessible at: $RHDH_URL" + curl -sI "$RHDH_URL" | head -5 +fi + +# Check logs for startup errors +kubectl logs deployment/$RHDH_DEPLOY -n $HELM_NS --tail=20 +``` + +### 4.6 Record rollback commands + +Record for Phase 7. Do not present them yet. + +```bash +helm rollback $HELM_RELEASE $HELM_REVISION -n $HELM_NS +``` + +--- + +## Phase 5: Generate Review Checklist + +Analyze the diff from Phase 1 and categorize changed files: + +| File pattern | Category | Review focus | +|-------------|----------|--------------| +| `charts/backstage/templates/` | Helm Templates | Resource definitions, conditionals, helper usage | +| `values.yaml` | Values/Config | Default values, breaking changes, schema alignment | +| `values.schema.tmpl.json`, `values.schema.json` | JSON Schema | OpenShift form compatibility, required fields | +| `Chart.yaml`, `Chart.lock` | Chart Metadata | Version bump, dependency updates | +| `charts/backstage/vendor/` | Upstream Subchart | Upstream sync, compatibility with wrapper chart | +| `charts/backstage/templates/tests/` | Chart Tests | Helm test coverage | +| `ct-*.yaml`, `.pre-commit-config.yaml` | CI/Tooling | Lint and test config changes | +| `.github/workflows/` | Build/CI | Workflow changes, release triggers | +| `docs/`, `README.md`, `*.gotmpl` | Documentation | Accuracy, helm-docs template sync | +| `.rhdh/` | RHDH-specific | Install scripts, CI integration | +| `charts/must-gather/`, `charts/orchestrator-*` | Other Charts | Separate chart changes (non-backstage) | + +### Generate the checklist + +For each category with changes, generate specific verification items. + +**Always include these baseline checks:** + +```markdown +### Baseline Checks +- [ ] RHDH pods started successfully with PR chart (no crash loops) +- [ ] RHDH logs show no errors (`kubectl logs deployment/$RHDH_DEPLOY -n $HELM_NS --tail=50`) +- [ ] RHDH UI is accessible via route/ingress +- [ ] Dynamic plugins init container completed successfully +``` + +**Template changes — add:** + +```markdown +### Template Verification +- [ ] `helm template` renders without errors +- [ ] Changed templates produce correct Kubernetes resources (`helm template | kubectl apply --dry-run=client -f -`) +- [ ] Conditional logic handles both enabled and disabled paths +- [ ] Helper functions in `_helpers.tpl` are used consistently +- [ ] Network policies (if changed) allow required traffic +``` + +**Values/Config changes — add:** + +```markdown +### Values Verification +- [ ] New values have sensible defaults in `values.yaml` +- [ ] Values are documented with `# --` comments (helm-docs format) +- [ ] Existing values are backward compatible (no breaking renames) +- [ ] `upstream:` prefixed values correctly pass through to subchart +- [ ] Dynamic plugin configuration works (`global.dynamic.plugins`) +``` + +**JSON Schema changes — add:** + +```markdown +### Schema Verification +- [ ] `values.schema.tmpl.json` template is updated (not just the generated file) +- [ ] `pre-commit run jsonschema-dereference` regenerates `values.schema.json` cleanly +- [ ] Schema validates against actual values.yaml defaults +- [ ] OpenShift console form renders correctly with new schema fields +``` + +**Chart metadata changes — add:** + +```markdown +### Chart Metadata +- [ ] Chart version bumped in `Chart.yaml` per Semantic Versioning +- [ ] Dependency versions are compatible (`Chart.lock` updated) +- [ ] `kubeVersion` constraint is correct +``` + +**Upstream subchart changes — add:** + +```markdown +### Upstream Subchart +- [ ] Changes in `vendor/backstage/` are from an upstream sync (not manual edits) +- [ ] Wrapper chart values still override subchart correctly +- [ ] Custom templates don't conflict with upstream template names +``` + +**Documentation changes — add:** + +```markdown +### Documentation +- [ ] `README.md` is regenerated via `pre-commit run helm-docs` +- [ ] `README.md.gotmpl` template is updated if new sections are needed +- [ ] Docs reflect the actual values and defaults +``` + +**End the checklist with:** + +```markdown +### Rollback +When done testing, rollback the chart: +[rollback command from Phase 4.6] +``` + +--- + +## Phase 6: Active Verification + +Typical chart verification actions: apply values overrides, check rendered resources, curl endpoints, verify ConfigMap data, test toggle behavior (enabled/disabled), check network policies. + +Read `../references/shared-verification.md` and follow the `` instructions, applying them to the specific chart changes in this PR. + +--- + +## Phase 7: Findings & Recommendations + +Read `../references/shared-findings-structure.md` and follow the `` instructions, using the chart-specific context below. + +### Chart best-practice bullets (for 7.2) + +- Does the change follow the subchart architecture (upstream values under `upstream:` key)? +- Are dynamic plugin configurations handled via `global.dynamic.includes` / `global.dynamic.plugins`? +- Is Route vs Ingress detection handled correctly for both OpenShift and vanilla K8s? +- Are backend auth secrets auto-generated when `global.auth.backend.enabled: true`? +- Do new values follow the existing naming conventions and nesting patterns? +- Is the `_helpers.tpl` used for reusable template logic instead of inline duplication? +- Are values documented with `# --` comments so helm-docs generates correct README? + +### Chart security bullets (for 7.3) + +- Are secrets handled safely (no plaintext in values.yaml defaults, proper Secret resources)? +- Are new network policies restrictive enough? +- Do new ServiceAccount configurations avoid unnecessary permissions? + +### Chart rollback commands (for 7.5) + +```bash +helm rollback $HELM_RELEASE $HELM_REVISION -n $HELM_NS +helm history $HELM_RELEASE -n $HELM_NS --max 3 +kubectl rollout status deployment/$RHDH_DEPLOY -n $HELM_NS --timeout=300s +``` + + + + + +| Trigger | Type | What | Resume When | +|---------|------|------|-------------| +| `helm template` fails | Stop | Chart has rendering errors | PR author fixes template syntax | +| Chart.yaml version not bumped | Warn | PR checklist requires version bump | Author updates Chart.yaml | +| No cluster access | Stop | User needs to `oc login` or configure kubeconfig | User logs in and re-runs skill | +| No RHDH Helm release | Deploy | Deploy RHDH via rhdh-test-instance `make deploy-helm` | RHDH pods are running | +| OLM-managed RHDH found | Redirect | Wrong deployment type for chart review | Use `review-operator-pr` workflow or deploy Helm-based RHDH in separate namespace | + + + + + +## Activity Logging + +```bash +$RHDH log add "Review PR # (rhdh-chart): deployed PR chart from branch , generated checklist" \ + --tag review-pr --tag rhdh-chart + +$RHDH log add "PR # active verification: , results: " \ + --tag review-pr --tag rhdh-chart + +$RHDH log add "PR # review findings: " \ + --tag review-pr --tag rhdh-chart +``` + +## Follow-up Todos + +```bash +$RHDH todo add "Follow up on PR # finding: " --context "review-pr" + +$RHDH todo add "Rollback chart on cluster after PR # review" --context "review-pr" +``` + + + + + +Review is complete when: + +- [ ] PR branch checked out and Helm dependencies updated +- [ ] Local validation passed (`helm template`, Chart.yaml version bump check) +- [ ] Cluster has RHDH deployed from PR chart (full chart, not just values override) +- [ ] RHDH pods are running and healthy (no crash loops) +- [ ] RHDH UI is accessible via route/ingress +- [ ] Review checklist generated from diff analysis +- [ ] Active verification plan proposed and accepted by user +- [ ] Verification executed with evidence captured +- [ ] Findings summary with pass/fail +- [ ] Best practice and security assessment completed +- [ ] Rollback instructions documented and shared with user +- [ ] Activity logged + + diff --git a/skills/rhdh-pr-review/workflows/review-operator-pr.md b/skills/rhdh-pr-review/workflows/review-operator-pr.md index 4a6aa01..58b784c 100644 --- a/skills/rhdh-pr-review/workflows/review-operator-pr.md +++ b/skills/rhdh-pr-review/workflows/review-operator-pr.md @@ -6,8 +6,11 @@ Fetch a PR's CI-built images, deploy the full operator bundle or manifests into Read these reference files before starting: -1. `../references/operator-pr-images.md` — Image naming, extraction, validation -2. `../../rhdh/references/github-reference.md` — gh CLI patterns +1. `../references/shared-pr-fetch.md` — Shared Phase 1 (PR context fetching) +2. `../references/shared-verification.md` — Shared Phase 6 (active verification) +3. `../references/shared-findings-structure.md` — Shared Phase 7 (findings structure) +4. `../references/operator-pr-images.md` — Image naming, extraction, validation +5. `../../rhdh/references/github-reference.md` — gh CLI patterns @@ -29,26 +32,9 @@ Read these reference files before starting: ```bash REPO="redhat-developer/rhdh-operator" PR_NUMBER= - -gh pr view $PR_NUMBER --repo $REPO \ - --json number,title,state,author,body,files,createdAt,headRefOid -``` - -Validate: -- PR state is `OPEN` (warn if merged or closed — images may still work but PR is not active) -- PR belongs to `redhat-developer/rhdh-operator` - -Fetch the diff for later checklist generation: - -```bash -gh pr diff $PR_NUMBER --repo $REPO ``` -Save the changed file list for Phase 5: - -```bash -gh pr view $PR_NUMBER --repo $REPO --json files --jq '.files[].path' -``` +Read `../references/shared-pr-fetch.md` and follow the `` instructions using the `REPO` and `PR_NUMBER` variables above. --- @@ -476,52 +462,17 @@ When done testing, rollback the operator image: ## Phase 6: Active Verification -**This phase verifies the PR's specific code changes on the cluster — not generic health checks.** The goal is to exercise the exact code paths the PR modified and capture evidence that the behavioral change works as intended. - -### 6.1 Analyze the diff - -Read the diff hunks from Phase 1. For each changed file, understand: - -- What the code did **before** the change -- What it does **after** -- What behavioral difference this introduces on a running cluster - -Map each changed code path to a concrete cluster-observable effect — something you can trigger and measure on the running cluster. If a code change has no cluster-observable effect (e.g., pure refactor with identical behavior), state that explicitly and explain why. - -### 6.2 Propose a verification plan - -Present the plan to the user. For each test, specify: - -- **What to do**: the exact cluster action (create resource, edit CR, delete pod, etc.) -- **What to observe**: where to look (logs, pod spec, CR status, events, API response) -- **Pass criteria**: what output means the fix works -- **Fail criteria**: what output means the fix is broken +Typical operator verification actions: create/edit Backstage CR, delete and recreate pods, check reconciliation logs, verify status conditions, inspect generated ConfigMaps/Secrets. -**STOP. Do not run any verification commands. Present the plan and wait for the user to accept it before proceeding to 6.3.** - -### 6.3 Execute the plan - -Only after the user accepts the plan: - -Run each verification step on the cluster. For every step, capture the actual command output as evidence. Do not summarize — show the raw output so the user can see exactly what happened. +Read `../references/shared-verification.md` and follow the `` instructions, applying them to the specific operator changes in this PR. --- ## Phase 7: Findings & Recommendations -Synthesize the verification results and provide a complete review assessment. - -### 7.1 Verification summary +Read `../references/shared-findings-structure.md` and follow the `` instructions, using the operator-specific context below. -Summarize what was tested and the results: - -| Category | Test performed | Result | Evidence | -|---|---|---|---| -| *[category]* | *[what was tested]* | Pass/Fail | *[key observation]* | - -### 7.2 Best practice assessment - -Review the PR's approach against operator development best practices. Reference `../../rhdh/references/rhdh-repos.md` for operator conventions: +### Operator best-practice bullets (for 7.2) - Does the change follow the existing reconciliation flow pattern (preprocess → init model → apply → cleanup → status)? - Are status conditions updated appropriately for new features or error cases? @@ -530,44 +481,23 @@ Review the PR's approach against operator development best practices. Reference - Are new CRD fields documented with appropriate kubebuilder markers? - Does the code avoid non-deterministic iteration patterns (sorted keys, stable ordering)? -### 7.3 Security review - -Evaluate the changes from a security perspective: +### Operator security bullets (for 7.3) -- Are new environment variables or secrets handled safely (no plaintext logging, proper RBAC)? -- Do RBAC changes follow least-privilege principle? -- Are container image references pinned by digest where appropriate? -- Are new network exposures (ports, routes, service accounts) intentional and documented? - Do dependency updates (`go.mod`) introduce known CVEs? -- Are user-supplied inputs validated before use in resource names or labels? -### 7.4 Improvement suggestions - -Based on the findings, suggest concrete improvements if any: - -- Code changes needed (reference specific files and lines from the diff) -- Missing test coverage for the changed code paths -- Documentation gaps -- Configuration or operational concerns - -### 7.5 Rollback instructions - -Present the rollback commands recorded in Phase 4.7: +### Operator rollback commands (for 7.5) **OLM-managed — restore original Subscription:** ```bash -# Delete PR-specific OLM resources oc delete subscription rhdh-operator-pr-subscription -n $OPERATOR_NS CSV_NAME=$(oc get csv -n $OPERATOR_NS --no-headers \ -o custom-columns=NAME:.metadata.name | grep rhdh) oc delete csv $CSV_NAME -n $OPERATOR_NS 2>/dev/null oc delete catalogsource rhdh-operator-pr-catalog -n $OPERATOR_NS -# Restore original Subscription (points back to the shared CatalogSource) oc apply -f /tmp/rollback-subscription.yaml -# Wait for OLM to redeploy the original operator oc wait csv -n $OPERATOR_NS -l "operators.coreos.com/$PACKAGE_NAME.$OPERATOR_NS=" \ --for=jsonpath='{.status.phase}'=Succeeded --timeout=180s ``` diff --git a/skills/rhdh/SKILL.md b/skills/rhdh/SKILL.md index c85c2a3..9a0956b 100644 --- a/skills/rhdh/SKILL.md +++ b/skills/rhdh/SKILL.md @@ -116,17 +116,18 @@ What would you like to do? *For testing PR changes on a live RHDH cluster* 8. **Review operator PR** — Deploy PR operator bundle on cluster and get review checklist +9. **Review chart PR** — Deploy PR chart on cluster and get review checklist ### Test Plan Tasks *For rhdh test plan review in jira* -9. **Review Test Plan content** — Reviews an RHDH test plan Jira ticket and suggests platform/integration version updates based on support lifecycle pages and RHDH release milestones +10. **Review Test Plan content** — Reviews an RHDH test plan Jira ticket and suggests platform/integration version updates based on support lifecycle pages and RHDH release milestones ### General Tasks -10. **Check environment** — Run doctor, configure paths -11. **View/search activity** — Review worklog, todos +11. **Check environment** — Run doctor, configure paths +12. **View/search activity** — Review worklog, todos **Wait for response before proceeding.** @@ -168,7 +169,7 @@ What would you like to do? | Response | Skill | |----------|-------| -| 8, "review PR", "rhdh-pr-review", "test PR", "operator PR", "swap image" | Route to `@rhdh-pr-review` skill | +| 8, 9, "review PR", "rhdh-pr-review", "test PR", "operator PR", "chart PR", "helm chart PR", "swap image" | Route to `@rhdh-pr-review` skill | **To route:** Read `../rhdh-pr-review/SKILL.md` and follow its intake process. @@ -176,7 +177,7 @@ What would you like to do? | Response | Skill | |----------|-------| -| 9, "review test plan", "update test plan", "check platform versions in test plan", "review RHDH test plan" | Route to `@rhdh-test-plan-review` skill | +| 10, "review test plan", "update test plan", "check platform versions in test plan", "review RHDH test plan" | Route to `@rhdh-test-plan-review` skill | **To route:** Read `../rhdh-test-plan-review/SKILL.md` and follow its intake process. @@ -184,8 +185,8 @@ What would you like to do? | Response | Action | |----------|--------| -| 10, "doctor", "setup", "config" | Use CLI commands below | -| 11, "log", "todo", "activity" | Use tracking commands below | +| 11, "doctor", "setup", "config" | Use CLI commands below | +| 12, "log", "todo", "activity" | Use tracking commands below |