# Reviewgen Legacy Feature

> Comprehensive review guide and criteria for PRs adding or modifying features in legacy resources (TF patches, DCL patches, CRD changes, MockGCP alignment, and Direct Controller alignment).

- Skill: `googlecloudplatform/reviewgen-legacy-feature` (Agent Skill)
- Install (CLI): `npx skillmds@latest add googlecloudplatform/reviewgen-legacy-feature`
- Raw SKILL.md: https://api.skillmd.com/api/skills/googlecloudplatform/reviewgen-legacy-feature/raw
- Safety review: pending
- Works with: Claude Code, Claude.ai, OpenAI Codex
- Category: Coding & Dev Tools
- Author: GoogleCloudPlatform (https://skillmd.com/u/googlecloudplatform)
- Updated: 2026-09-17
- Page: https://skillmd.com/skills/googlecloudplatform/reviewgen-legacy-feature

---


# Review Guide for Legacy Resource Feature Changes

This skill outlines the mandatory review criteria for PRs that introduce new fields, update behaviors, or backport patches for **Legacy resources** (Terraform-based or DCL-based resources). Note that the terms "legacy resource" and "brownfield resource" are used interchangeably throughout this skill.

---

## 1. Terraform (TF) Based Resource Changes

### Patch Verification
*   **Target Files**: Patch files are under `third_party/github.com/hashicorp/terraform-provider-google-beta/google-beta/services/[servicename]/resource_[servicename]_[typename].go` (or extracted schema files like `node_config.go`).
*   **Adding New Fields**:
    1. Verify schema addition in the schema map (`map[string]*schema.Schema`).
    2. Verify expanding logic (Terraform struct -> GCP SDK) is implemented for new fields.
    3. Verify flattening logic (GCP SDK -> Terraform struct) is implemented for new fields.
*   **Modifying Existing Fields (Backwards-Compatible Behavior Changes)**:
    1. Schema definition updated as required.
    2. Related expanding/flattening logic updated to match the modified schema.

### CRD & Service Mapping Changes
*   **Resource References**: If a new reference field is added, ensure it is properly configured in `config/servicemappings/[servicename].yaml` under `resourceReferences`.
*   **CRD Schema Alignment**: Verify that schema changes are reflected in the corresponding generated CRD file under `config/crds/resources/`.

---

## 2. Declarative Resource Client Library (DCL) Based Resource Changes

> [!IMPORTANT]
> Triple check if DCL changes are strictly necessary before approving. Direct migrations are preferred.

### Patch Verification
*   **Target Files**: Patch files are under `third_party/github.com/GoogleCloudPlatform/declarative-resource-client-library/services/google/[servicename]/`.
*   **Multi-Version Requirement**: If multiple versions exist for the type, changes **must** be made across all supported versions.
*   **Adding New Fields**:
    1. Schema file `[typename].yaml` updated with the new fields.
    2. Expanding logic added in `[typename]_internal.go`.
    3. Flattening logic added in `[typename]_internal.go`.
    4. Supporting logic updated in `[typename].go` or helper files.
*   **Modifying Existing Fields**:
    1. Schema file `[typename].yaml` updated.
    2. Implementation logic updated to match.

### CRD Schema Changes
*   Ensure reference fields are configured in `[typename].yaml`.
*   Verify CRD YAML changes are generated in `config/crds/resources/`.

---

## 3. Shared Review Invariants

### A. Test Coverage
*   **New Fields**: Must be covered in `create.yaml` and/or `update.yaml` within a dedicated fixture test directory under `pkg/test/resourcefixture/testdata/basic/`. Multiple test cases are required if fields are mutually exclusive explicitly (`oneOf`) or implicitly (errors from the service).
*   **Existing Fields / Behavior Changes**: New behavior or state transitions must be covered in `create.yaml` and/or `update.yaml`.

### B. MockGCP and Real GCP Alignment

1. **Determining Target Log Files & Workflow via `static_config.go`**:
   - Inspect `pkg/controller/resourceconfig/static_config.go` for the resource's `{Group, Kind}` entry under `ControllerConfigStatic`.
   - **Case 1: Unmigrated Dual-Controller Resource** (`DefaultController` is `Terraform` or `DCL`, but `SupportedControllers` includes `Direct`):
     - Target logs to compare for the TF/DCL patch: compare **`_http_old_controller.log`** vs **`_http_old_controller_mock.log`**.
   - **Case 2: Unmigrated Single-Controller Resource** (Only `Terraform` or `DCL` in `SupportedControllers`):
     - Target logs to compare for the TF/DCL patch: compare **`_http.log`** vs **`_http_mock.log`**.
   - **Case 3: Migrated Resource (Yellow Flag ⚠️)** (`DefaultController` is `Direct`):
     - **Warning**: It does not make sense to add or modify features via a legacy TF/DCL patch once the default controller has been cut over to `Direct`.
     - **Review Action**: Flag this in the review summary as a Yellow Flag. Instruct the contributor to follow the Direct Controller feature workflow instead (implementing the new fields/features directly in `pkg/controller/direct/` and `apis/`).

2. **Verification of Real GCP Test Execution (Mandatory First Step)**:
   * Ensure that the real GCP test has actually been executed.
   * **Detection Heuristics**:
     - Look for expected minor/unimportant diffs between real logs and mock logs (e.g., dynamic server timestamps, IDs, or volatile hashes).
     - **Identical Logs Check**: If real and mock logs are 100% identical with no minor/dynamic diffs, flag this in the review to check with the contributor to verify if the real GCP test (`hack/record-gcp`) was actually run (or if mock logs were merely copied).
     - Or confirm if the human contributor explicitly states in PR comments/description that they ran the real GCP test.
   * **Bot-Submitted PRs**: If the PR is submitted by a code bot (e.g., `lovelace-coder-bot`), explicitly ask the human reviewer to verify if the bot has run the real GCP test (providing the time range during which the real test could have been running).

3. **Mock Support**: MockGCP support for the updated brownfield resource must be implemented or verified to run tests hermetically.

4. **HTTP Log Diffs & Verification Rules**:
   * **Single Controller (Unmigrated)**: Compare real GCP HTTP logs (`_http.log`) against mock GCP HTTP logs (`_http_mock.log`).
   * **Dual Controller (Unmigrated)**: Compare legacy real GCP logs (`_http_old_controller.log`) against legacy mock logs (`_http_old_controller_mock.log`).
   * **Strict Rule**: Verify that real vs mock log diffs **do not contain discrepancies in the newly modified/added fields**. Any discrepancy indicates a bug or gap in the MockGCP implementation, but we'll only focus on the discrepancies that impact the target feature in the PR.
   * **`golden_alignment_test.go` Test Coverage Rule**: Automated log comparison is executed by `pkg/test/resourcefixture/golden_alignment_test.go`. If the review identifies unexpected log diffs or discrepancies in the target feature that `golden_alignment_test.go` missed, updating the test file is **out of scope** for the feature PR; instead, **`reviewgen` should create a GitHub issue** (e.g. via `gh issue create`) to capture this test gap and report the issue back in the review summary.

5. **KRM Golden Object File Verification (`_generated_object_<testname>.golden.yaml`)**:
   * **Dual-Controller Object Diffs**: For unmigrated dual-controller resources, check `_final_object_old_controller.golden.yaml` against `_generated_object_<testname>.golden.yaml` (and inspect `_final_object.diff`) to ensure status alignment between the legacy controller and direct controller.

### C. Direct Controller Alignment
1. **Adding New Fields**:
   * **Direct Types, Mappers & Fuzzers**: If the brownfield resource already has a direct type (`apis/`), mapper (`pkg/controller/direct/`), and/or fuzzer, ensure changes are added to those files.
   * **Reference Types**: If the new field requires a reference type that was not already implemented, the reference type must be added.
   * **Direct Controller `Update()` Logic**: If the resource already has a direct controller (`_controller.go`), verify if special handling is needed in `Update()`:
     - *Value Equivalence Diffing*: If the desired value and actual value of the new field are formatted differently but mean the same thing, the diffing logic in `Update()` must be updated to recognize equivalence.
     - *Field Comparison Lists*: When `Update()` compares desired and actual resources based on explicit field names, add the new fields to the comparison list.
2. **Modifying Existing Fields (Backwards-Compatibility Changes)**:
   * **Direct Types**: If the brownfield resource already has direct types in `apis/`, update the type files to reflect the schema changes.
   * **Direct Mappers, Fuzzers & Controllers**: If the brownfield resource already has a direct mapper, fuzzer, or controller, determine if updates are required:
     - *Immutability Check Removal*: If a field becomes mutable, check the controller logic and remove any obsolete immutability validation checks that would prevent in-place updates.
     - *Mapper/Fuzzer Alignment*: Update mappers and fuzzers to ensure proper translation and test coverage for the modified field.
3. **Dual-Controller Traffic Alignment Verification**:
   * For an **Unmigrated Dual-Controller Resource**, after verifying code/type changes, inspect both real logs (**`_http_old_controller.log`** vs **`_http.log`**, `_http.diff`) and mock logs (**`_http_old_controller_mock.log`** vs **`_http_mock.log`**, `_http_mock.diff`) to ensure traffic aligns for the newly added/modified fields.
   * Discrepancies between legacy controller traffic and direct controller traffic for the target feature indicate that the direct mapper/controller was not properly updated or handles the new field differently.

---

## Standard Review Summary Template

```markdown
### Legacy Feature Review Results

* **Real GCP Test Verification**: [Pass/Pending/Needs-Human-Verification] - (Verified via minor _http.log vs _http_mock.log diffs or contributor claim. For bot PRs: prompt human reviewer with timestamp window)
* **TF / DCL Patch Integrity**: [Pass/Fail] - (Verified schema, expanders, and flatteners)
* **CRD & Service Mapping**: [Pass/Fail] - (CRD and servicemapping updated)
* **Test Fixture Coverage**: [Pass/Fail] - (Verified create.yaml / update.yaml coverage)
* **MockGCP and Real GCP Alignment**: [Pass/Fail] - (No unexpected diffs in new fields in _http.log vs _http_mock.log)
* **Direct Controller Alignment**: [Pass/Fail] - (Updated apis/, mappers, fuzzers, and immutability checks if present)

#### Actionable Findings:
1. [File line number and specific issue]
```