fix(add-patch): Make patch discovery work across whole spec#18
fix(add-patch): Make patch discovery work across whole spec#18reubeno merged 1 commit intomicrosoft:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes patch overlay behavior by making patch tag discovery/removal operate across the entire RPM spec (all packages) to avoid PatchN collisions when sub-packages define their own Patch entries.
Changes:
- Change patch tag scanning to find the highest PatchN across the whole spec (not per-package).
- Update patch removal to remove matching PatchN tags across all packages (and update overlay call sites).
- Add/adjust tests and introduce
VisitTags/VisitTagsPackagesplit to support both global and package-scoped traversal.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| internal/rpm/spec/edit.go | Updates traversal APIs and patch add/remove logic to operate across the whole spec for PatchN discovery/removal. |
| internal/rpm/spec/edit_test.go | Updates existing tests for new semantics and adds coverage for tag visiting and multi-package patch removal. |
| internal/app/azldev/core/sources/overlays.go | Updates overlay application to use the new RemovePatchEntry signature. |
internal/rpm/spec/edit.go
Outdated
|
|
||
| matched, matchErr := doublestar.Match(pattern, tagLine.Value) | ||
| if matchErr != nil { | ||
| return nil |
There was a problem hiding this comment.
Errors from doublestar.Match (e.g., invalid glob patterns) are currently swallowed, which can lead to misleading outcomes (such as returning "no patches matching" when the real issue is an invalid pattern). Propagate matchErr so callers get a clear failure reason (or wrap it with context including the pattern).
| return nil | |
| return fmt.Errorf("failed to match glob pattern %#q against %#q:\n%w", pattern, tagLine.Value, matchErr) |
6916d5e to
afd4b4e
Compare
|
Changes look good, @binujp. We'll get it merged after the latest round of PR checks pass. |
"patch-add" overlay type assumes patch numbering is per-package section. This means a patchX defined inside a sub-package can be identified as the highest patch number. This can cause a collision with another patchX defined elsewhere in the spec. This PR makes patch-add look for highest existing patchN across the spec, the same for patch-remove and also some tests to verify the funcitonality. To avoid overloading existing methods new ones were added. While this is a few extra lines of code, maintenance and feature addition will become much easier.
afd4b4e to
fe2e646
Compare
| } | ||
| case projectconfig.ComponentOverlayRemovePatch: | ||
| err := openedSpec.RemovePatchEntry(overlay.PackageName, overlay.Filename) | ||
| err := openedSpec.RemovePatchEntry(overlay.Filename) |
There was a problem hiding this comment.
patch-remove overlays now call openedSpec.RemovePatchEntry(overlay.Filename) and ignore overlay.PackageName, so any package = "..." setting in config will have no effect. Either restore package-scoped removal (e.g., keep a package-aware API and call it here) or update the user docs/schema to explicitly state that patch-remove always scans/removes across the whole spec regardless of package to avoid surprising users.
| err := openedSpec.RemovePatchEntry(overlay.Filename) | |
| err := openedSpec.RemovePatchEntry(overlay.PackageName, overlay.Filename) |
"patch-add" overlay type assumes patch numbering is per-package section. This means a patchX defined inside a sub-package can be identified as the highest patch number. This can cause a collision with another patchX defined elsewhere in the spec.
This PR makes patch-add look for highest existing patchN across the spec, the same for patch-remove and also some tests to verify the funcitonality. To avoid overloading existing methods new ones were added. While this is a few extra lines of code, maintenance and feature addition will become much easier.