|
| 1 | +# review-breaking-changes |
| 2 | + |
| 3 | +Breaking change detection rules for connector reviews. |
| 4 | + |
| 5 | +--- |
| 6 | + |
| 7 | +## What Breaks Downstream |
| 8 | + |
| 9 | +When a connector changes, these break C1 sync: |
| 10 | + |
| 11 | +| Change | Impact | Severity | |
| 12 | +|--------|--------|----------| |
| 13 | +| Resource type name change | Existing grants orphaned | Critical | |
| 14 | +| Entitlement slug change | Grant matching fails | Critical | |
| 15 | +| Resource ID format change | Duplicate resources created | Critical | |
| 16 | +| Removed resource type | Grants disappear | High | |
| 17 | +| Changed parent hierarchy | Relationships break | High | |
| 18 | +| Removed entitlement | Grants can't be revoked | High | |
| 19 | +| Changed trait type | C1 UI breaks | Medium | |
| 20 | + |
| 21 | +--- |
| 22 | + |
| 23 | +## Resource Type Changes |
| 24 | + |
| 25 | +**Breaking - type name:** |
| 26 | +```go |
| 27 | +// BEFORE |
| 28 | +var userResourceType = &v2.ResourceType{ |
| 29 | + Id: "user", // This is the identity |
| 30 | + DisplayName: "User", |
| 31 | +} |
| 32 | + |
| 33 | +// AFTER - BREAKING |
| 34 | +var userResourceType = &v2.ResourceType{ |
| 35 | + Id: "account", // Changed! All grants orphaned |
| 36 | + DisplayName: "User Account", |
| 37 | +} |
| 38 | +``` |
| 39 | + |
| 40 | +**Safe - display name only:** |
| 41 | +```go |
| 42 | +// BEFORE |
| 43 | +var userResourceType = &v2.ResourceType{ |
| 44 | + Id: "user", |
| 45 | + DisplayName: "User", |
| 46 | +} |
| 47 | + |
| 48 | +// AFTER - Safe |
| 49 | +var userResourceType = &v2.ResourceType{ |
| 50 | + Id: "user", // Same |
| 51 | + DisplayName: "User Account", // Display change is fine |
| 52 | +} |
| 53 | +``` |
| 54 | + |
| 55 | +--- |
| 56 | + |
| 57 | +## Entitlement Changes |
| 58 | + |
| 59 | +**Breaking - slug change:** |
| 60 | +```go |
| 61 | +// BEFORE |
| 62 | +sdkEntitlement.NewAssignmentEntitlement(resource, "member", ...) |
| 63 | + |
| 64 | +// AFTER - BREAKING |
| 65 | +sdkEntitlement.NewAssignmentEntitlement(resource, "membership", ...) |
| 66 | +``` |
| 67 | + |
| 68 | +The slug `"member"` vs `"membership"` breaks grant matching. |
| 69 | + |
| 70 | +**Breaking - permission entitlement change:** |
| 71 | +```go |
| 72 | +// BEFORE |
| 73 | +sdkEntitlement.NewPermissionEntitlement(resource, "admin", ...) |
| 74 | + |
| 75 | +// AFTER - BREAKING |
| 76 | +sdkEntitlement.NewPermissionEntitlement(resource, "administrator", ...) |
| 77 | +``` |
| 78 | + |
| 79 | +--- |
| 80 | + |
| 81 | +## Resource ID Changes |
| 82 | + |
| 83 | +**Breaking - ID derivation:** |
| 84 | +```go |
| 85 | +// BEFORE |
| 86 | +rs.NewUserResource(user.Name, userType, user.ID, ...) |
| 87 | + |
| 88 | +// AFTER - BREAKING |
| 89 | +rs.NewUserResource(user.Name, userType, user.Email, ...) |
| 90 | +``` |
| 91 | + |
| 92 | +IDs must be stable. Changing how they're derived creates duplicates. |
| 93 | + |
| 94 | +**Breaking - ID format:** |
| 95 | +```go |
| 96 | +// BEFORE |
| 97 | +resourceID := user.ID |
| 98 | + |
| 99 | +// AFTER - BREAKING |
| 100 | +resourceID := fmt.Sprintf("user:%s", user.ID) |
| 101 | +``` |
| 102 | + |
| 103 | +--- |
| 104 | + |
| 105 | +## Hierarchy Changes |
| 106 | + |
| 107 | +**Breaking - parent resource change:** |
| 108 | +```go |
| 109 | +// BEFORE - users under organization |
| 110 | +rs.NewUserResource(name, userType, id, []rs.UserTraitOption{}, |
| 111 | + rs.WithParentResourceID(&v2.ResourceId{ |
| 112 | + ResourceType: "organization", |
| 113 | + Resource: orgID, |
| 114 | + }), |
| 115 | +) |
| 116 | + |
| 117 | +// AFTER - BREAKING - users under workspace |
| 118 | +rs.NewUserResource(name, userType, id, []rs.UserTraitOption{}, |
| 119 | + rs.WithParentResourceID(&v2.ResourceId{ |
| 120 | + ResourceType: "workspace", // Changed parent type |
| 121 | + Resource: wsID, |
| 122 | + }), |
| 123 | +) |
| 124 | +``` |
| 125 | + |
| 126 | +--- |
| 127 | + |
| 128 | +## Trait Changes |
| 129 | + |
| 130 | +**Breaking - trait type change:** |
| 131 | +```go |
| 132 | +// BEFORE - User trait |
| 133 | +rs.NewUserResource(name, userType, id, []rs.UserTraitOption{ |
| 134 | + rs.WithEmail(email, true), |
| 135 | +}) |
| 136 | + |
| 137 | +// AFTER - BREAKING - App trait |
| 138 | +rs.NewAppResource(name, appType, id, []rs.AppTraitOption{}) |
| 139 | +``` |
| 140 | + |
| 141 | +**Safe - adding trait options:** |
| 142 | +```go |
| 143 | +// BEFORE |
| 144 | +rs.NewUserResource(name, userType, id, []rs.UserTraitOption{ |
| 145 | + rs.WithEmail(email, true), |
| 146 | +}) |
| 147 | + |
| 148 | +// AFTER - Safe |
| 149 | +rs.NewUserResource(name, userType, id, []rs.UserTraitOption{ |
| 150 | + rs.WithEmail(email, true), |
| 151 | + rs.WithUserLogin(login), // Adding is safe |
| 152 | +}) |
| 153 | +``` |
| 154 | + |
| 155 | +--- |
| 156 | + |
| 157 | +## Detection Patterns |
| 158 | + |
| 159 | +**Search for these in diffs:** |
| 160 | + |
| 161 | +``` |
| 162 | +# Resource type ID changes |
| 163 | +- Id: " |
| 164 | ++ Id: " |
| 165 | +
|
| 166 | +# Entitlement slug changes |
| 167 | +- NewAssignmentEntitlement(resource, " |
| 168 | ++ NewAssignmentEntitlement(resource, " |
| 169 | +
|
| 170 | +- NewPermissionEntitlement(resource, " |
| 171 | ++ NewPermissionEntitlement(resource, " |
| 172 | +
|
| 173 | +# Resource ID derivation |
| 174 | +- rs.NewUserResource(*, *, user.ID, |
| 175 | ++ rs.NewUserResource(*, *, user.Email, |
| 176 | +
|
| 177 | +# Parent resource changes |
| 178 | +- ResourceType: "organization", |
| 179 | ++ ResourceType: "workspace", |
| 180 | +``` |
| 181 | + |
| 182 | +--- |
| 183 | + |
| 184 | +## CodeRabbit/BUGBOT Rules |
| 185 | + |
| 186 | +For automated detection, configure these patterns: |
| 187 | + |
| 188 | +```yaml |
| 189 | +# .coderabbit.yaml or BUGBOT.md |
| 190 | +reviews: |
| 191 | + path_filters: |
| 192 | + - "pkg/**/*.go" |
| 193 | + - "cmd/**/*.go" |
| 194 | + |
| 195 | + high_severity_patterns: |
| 196 | + - pattern: 'Id:\s*"[^"]+"\s*,\s*//.*change' |
| 197 | + message: "Resource type ID change detected - this breaks existing grants" |
| 198 | + |
| 199 | + - pattern: 'NewAssignmentEntitlement\([^,]+,\s*"[^"]+"' |
| 200 | + message: "Entitlement slug change - verify this doesn't break grant matching" |
| 201 | + |
| 202 | + - pattern: 'NewPermissionEntitlement\([^,]+,\s*"[^"]+"' |
| 203 | + message: "Permission entitlement change - verify slug stability" |
| 204 | +``` |
| 205 | +
|
| 206 | +--- |
| 207 | +
|
| 208 | +## Review Questions |
| 209 | +
|
| 210 | +When reviewing changes to resource types or entitlements: |
| 211 | +
|
| 212 | +1. **"Does this change any identifier that C1 uses for matching?"** |
| 213 | + - Resource type ID |
| 214 | + - Entitlement slug |
| 215 | + - Resource ID derivation |
| 216 | +
|
| 217 | +2. **"If this deploys, what happens to existing data?"** |
| 218 | + - Existing grants matched by old identifiers |
| 219 | + - Users/groups with old resource IDs |
| 220 | +
|
| 221 | +3. **"Is there a migration path?"** |
| 222 | + - Can old and new coexist temporarily? |
| 223 | + - Is there a way to map old IDs to new? |
| 224 | +
|
| 225 | +--- |
| 226 | +
|
| 227 | +## Safe Changes |
| 228 | +
|
| 229 | +These are NOT breaking: |
| 230 | +
|
| 231 | +- Display name changes (human-readable only) |
| 232 | +- Description changes |
| 233 | +- Adding new resource types |
| 234 | +- Adding new entitlements |
| 235 | +- Adding new trait options to existing resources |
| 236 | +- Adding new fields to API responses |
| 237 | +- Improving error messages |
| 238 | +- Performance optimizations |
| 239 | +- Adding pagination to unpaginated endpoints |
| 240 | +
|
| 241 | +--- |
| 242 | +
|
| 243 | +## Migration Guidance |
| 244 | +
|
| 245 | +If a breaking change is necessary: |
| 246 | +
|
| 247 | +1. **Document the break** in PR description |
| 248 | +2. **Coordinate with C1 team** for sync timing |
| 249 | +3. **Consider dual-emit** temporarily (old + new IDs) |
| 250 | +4. **Plan grant re-sync** after deployment |
| 251 | +5. **Update any hardcoded references** in C1 config |
0 commit comments