|
| 1 | +--- |
| 2 | +applyTo: "**/*" |
| 3 | +--- |
| 4 | + |
| 5 | +# Code Review Branch Strategy Guide |
| 6 | + |
| 7 | +This guide helps GitHub Copilot provide appropriate feedback when reviewing code changes, particularly distinguishing between issues that should be fixed in the current branch versus the default branch. |
| 8 | + |
| 9 | +## Purpose |
| 10 | + |
| 11 | +When reviewing pull requests, especially those targeting release branches, it's important to identify whether an issue should be fixed in: |
| 12 | +- **The current PR/branch** - Release-specific fixes or backports |
| 13 | +- **The default branch first** - General bugs that exist in the main codebase |
| 14 | + |
| 15 | +## Branch Types and Fix Strategy |
| 16 | + |
| 17 | +### Release Branches (e.g., `release/v7.5`, `release/v7.4`) |
| 18 | + |
| 19 | +**Purpose:** Contain release-specific changes and critical backports |
| 20 | + |
| 21 | +**Should contain:** |
| 22 | +- Release-specific configuration changes |
| 23 | +- Critical bug fixes that are backported from the default branch |
| 24 | +- Release packaging/versioning adjustments |
| 25 | + |
| 26 | +**Should NOT contain:** |
| 27 | +- New general bug fixes that haven't been fixed in the default branch |
| 28 | +- Refactoring or improvements that apply to the main codebase |
| 29 | +- Workarounds for issues that exist in the default branch |
| 30 | + |
| 31 | +### Default/Main Branch (e.g., `master`, `main`) |
| 32 | + |
| 33 | +**Purpose:** Primary development branch for all ongoing work |
| 34 | + |
| 35 | +**Should contain:** |
| 36 | +- All general bug fixes |
| 37 | +- New features and improvements |
| 38 | +- Refactoring and code quality improvements |
| 39 | +- Fixes that will later be backported to release branches |
| 40 | + |
| 41 | +## Identifying Issues That Belong in the Default Branch |
| 42 | + |
| 43 | +When reviewing a PR targeting a release branch, look for these indicators that suggest the fix should be in the default branch first: |
| 44 | + |
| 45 | +### 1. The Root Cause Exists in Default Branch |
| 46 | + |
| 47 | +If the underlying issue exists in the default branch's code, it should be fixed there first. |
| 48 | + |
| 49 | +**Example:** |
| 50 | +```yaml |
| 51 | +# PR changes this in release/v7.5: |
| 52 | +- $metadata = Get-Content "$repoRoot/tools/metadata.json" -Raw | ConvertFrom-Json |
| 53 | ++ $metadata = Get-Content "$(Build.SourcesDirectory)/PowerShell/tools/metadata.json" -Raw | ConvertFrom-Json |
| 54 | +``` |
| 55 | + |
| 56 | +**Analysis:** If `$repoRoot` is undefined because the template doesn't include its dependencies in BOTH the release branch AND the default branch, the fix should address the root cause in the default branch first. |
| 57 | + |
| 58 | +### 2. The Fix is a Workaround Rather Than a Proper Solution |
| 59 | + |
| 60 | +If the change introduces a workaround (hardcoded paths, special cases) rather than fixing the underlying design issue, it likely belongs in the default branch as a proper fix. |
| 61 | + |
| 62 | +**Example:** |
| 63 | +- Using hardcoded paths instead of fixing variable initialization |
| 64 | +- Adding special cases instead of fixing the logic |
| 65 | +- Duplicating code instead of fixing shared dependencies |
| 66 | + |
| 67 | +### 3. The Issue Affects General Functionality |
| 68 | + |
| 69 | +If the issue affects general functionality not specific to a release, it should be fixed in the default branch. |
| 70 | + |
| 71 | +**Example:** |
| 72 | +- Template dependencies that affect all pipelines |
| 73 | +- Shared utility functions |
| 74 | +- Common configuration issues |
| 75 | + |
| 76 | +## Providing Code Review Feedback |
| 77 | + |
| 78 | +### For Issues in the Current Branch |
| 79 | + |
| 80 | +When an issue is specific to the current branch or is a legitimate fix for the branch being targeted, **use the default code review feedback format** without any special branch-strategy commentary. |
| 81 | + |
| 82 | +### For Issues That Belong in the Default Branch |
| 83 | + |
| 84 | +1. **Provide the code review feedback** |
| 85 | +2. **Explain why it should be fixed in the default branch** |
| 86 | +3. **Provide an issue template** in markdown format |
| 87 | + |
| 88 | +**Example:** |
| 89 | + |
| 90 | +```markdown |
| 91 | +The `channelSelection.yml` template relies on `$repoRoot` being set by `SetVersionVariables.yml`, but doesn't declare this dependency. This issue exists in both the release branch and the default branch. |
| 92 | + |
| 93 | +**This should be fixed in the default branch first**, then backported if needed. The proper fix is to ensure template dependencies are correctly declared, rather than using hardcoded paths as a workaround. |
| 94 | + |
| 95 | +--- |
| 96 | + |
| 97 | +**Suggested Issue for Default Branch:** |
| 98 | + |
| 99 | +### Issue Title |
| 100 | +`channelSelection.yml` template missing dependency on `SetVersionVariables.yml` |
| 101 | + |
| 102 | +### Description |
| 103 | +The `channelSelection.yml` template uses the `$repoRoot` variable but doesn't ensure it's set beforehand by including `SetVersionVariables.yml`. |
| 104 | + |
| 105 | +**Current State:** |
| 106 | +- `channelSelection.yml` expects `$repoRoot` to be available |
| 107 | +- Not all pipelines that use `channelSelection.yml` include `SetVersionVariables.yml` first |
| 108 | +- This creates an implicit dependency that's not enforced |
| 109 | + |
| 110 | +**Expected State:** |
| 111 | +Either: |
| 112 | +1. `channelSelection.yml` should include `SetVersionVariables.yml` as a dependency, OR |
| 113 | +2. `channelSelection.yml` should be refactored to not depend on `$repoRoot`, OR |
| 114 | +3. Pipelines using `channelSelection.yml` should explicitly include `SetVersionVariables.yml` first |
| 115 | + |
| 116 | +**Files Affected:** |
| 117 | +- `.pipelines/templates/channelSelection.yml` |
| 118 | +- `.pipelines/templates/package-create-msix.yml` |
| 119 | +- `.pipelines/templates/release-SetTagAndChangelog.yml` |
| 120 | + |
| 121 | +**Priority:** Medium |
| 122 | +**Labels:** `Issue-Bug`, `Area-Build`, `Area-Pipeline` |
| 123 | +``` |
| 124 | + |
| 125 | +## Issue Template Format |
| 126 | + |
| 127 | +When creating an issue template for the default branch, use this structure: |
| 128 | + |
| 129 | +```markdown |
| 130 | +### Issue Title |
| 131 | +[Clear, concise description of the problem] |
| 132 | + |
| 133 | +### Description |
| 134 | +[Detailed explanation of the issue] |
| 135 | + |
| 136 | +**Current State:** |
| 137 | +- [What's happening now] |
| 138 | +- [Why it's problematic] |
| 139 | + |
| 140 | +**Expected State:** |
| 141 | +- [What should happen] |
| 142 | +- [Proposed solution(s)] |
| 143 | + |
| 144 | +**Files Affected:** |
| 145 | +- [List of files] |
| 146 | + |
| 147 | +**Priority:** [Low/Medium/High/Critical] |
| 148 | +**Labels:** [Suggested labels like `Issue-Bug`, `Area-*`] |
| 149 | + |
| 150 | +**Additional Context:** |
| 151 | +[Any additional information, links to related issues, etc.] |
| 152 | +``` |
| 153 | + |
| 154 | +## Common Scenarios |
| 155 | + |
| 156 | +### Scenario 1: Template Dependency Issues |
| 157 | + |
| 158 | +**Indicators:** |
| 159 | +- Missing template includes |
| 160 | +- Undefined variables from other templates |
| 161 | +- Assumptions about pipeline execution order |
| 162 | + |
| 163 | +**Action:** Suggest fixing template dependencies in the default branch. |
| 164 | + |
| 165 | +### Scenario 2: Hardcoded Values |
| 166 | + |
| 167 | +**Indicators:** |
| 168 | +- Hardcoded paths replacing variables |
| 169 | +- Environment-specific values in shared code |
| 170 | +- Magic strings or numbers |
| 171 | + |
| 172 | +**Action:** Suggest proper variable/parameter usage in the default branch. |
| 173 | + |
| 174 | +### Scenario 3: Logic Errors |
| 175 | + |
| 176 | +**Indicators:** |
| 177 | +- Incorrect conditional logic |
| 178 | +- Missing error handling |
| 179 | +- Race conditions |
| 180 | + |
| 181 | +**Action:** Suggest fixing the logic in the default branch unless it's release-specific. |
| 182 | + |
| 183 | +### Scenario 4: Legitimate Release Branch Fixes |
| 184 | + |
| 185 | +**Indicators:** |
| 186 | +- Version-specific configuration |
| 187 | +- Release packaging changes |
| 188 | +- Backport of already-fixed default branch issue |
| 189 | + |
| 190 | +**Action:** Provide normal code review feedback for the current PR. |
| 191 | + |
| 192 | +## Best Practices |
| 193 | + |
| 194 | +1. **Always check if the issue exists in the default branch** before suggesting a release-branch-only fix |
| 195 | +2. **Prefer fixing root causes over workarounds** |
| 196 | +3. **Provide clear rationale** for why a fix belongs in the default branch |
| 197 | +4. **Include actionable issue templates** so users can easily create issues |
| 198 | +5. **Be helpful, not blocking** - provide the feedback even if you can't enforce where it's fixed |
| 199 | + |
| 200 | +## Examples of Good vs. Bad Approaches |
| 201 | + |
| 202 | +### ❌ Bad: Workaround in Release Branch Only |
| 203 | + |
| 204 | +```yaml |
| 205 | +# In release/v7.5 only |
| 206 | +- pwsh: | |
| 207 | + $metadata = Get-Content "$(Build.SourcesDirectory)/PowerShell/tools/metadata.json" -Raw |
| 208 | +``` |
| 209 | +
|
| 210 | +**Why bad:** Hardcodes path to work around missing `$repoRoot`, doesn't fix the default branch. |
| 211 | + |
| 212 | +### ✅ Good: Fix in Default Branch, Then Backport |
| 213 | + |
| 214 | +```yaml |
| 215 | +# In default branch first |
| 216 | +- template: SetVersionVariables.yml@self # Ensures $repoRoot is set |
| 217 | +- template: channelSelection.yml@self # Now can use $repoRoot |
| 218 | +``` |
| 219 | + |
| 220 | +**Why good:** Fixes the root cause by ensuring dependencies are declared, then backport to release if needed. |
| 221 | + |
| 222 | +## When in Doubt |
| 223 | + |
| 224 | +If you're unsure whether an issue should be fixed in the current branch or the default branch, ask yourself: |
| 225 | + |
| 226 | +1. Does this issue exist in the default branch? |
| 227 | +2. Is this a workaround or a proper fix? |
| 228 | +3. Will other branches/releases benefit from this fix? |
| 229 | + |
| 230 | +If the answer to any of these is "yes," suggest fixing it in the default branch first. |
0 commit comments