Repository navigation
Add --only to filter GODEBUG changes by package - #6
snehaapratap wants to merge 1 commit into
Conversation
GillMacedo
left a comment
There was a problem hiding this comment.
One change needed: an empty --only value is ignored instead of rejected.
To reproduce, from the repo root:
go run ./plugin/scripts/godebugdiff.go --to 1.24 --only "" --fail examples/shop
go run ./plugin/scripts/godebugdiff.go --to 1.24 --only " " --fail examples/shop
The first prints the full unfiltered report (15 changes, no Packages: line) and exits 1. The second exits 2 with --only requires comma-separated non-empty package names. --only= takes the same path as the first, because filterPackages returns early on only == "" and can't tell an omitted flag from an empty value.
This matters in scripts: --only "$PKGS" --fail with an empty variable drops the filter without a word, so --fail can exit 1 on packages the caller meant to exclude. It also doesn't match the PR's statement that empty entries are rejected.
To fix: reject an explicitly set empty value with the same error and exit code 2, for example by using fs.Visit to detect whether only was set. Keep the unfiltered behavior when the flag is omitted. Add a case to the integration test, since TestFilterPackagesValidation only calls filterPackages and can't see this difference.
Checked and working:
make testpasses.- Exact matching, including that
net/httpexcludes other packages:--only "net/http, crypto/tls"onexamples/shopgives 5 of the 15 changes. - Whitespace and duplicate names are handled.
--onlyworks before and after the directory.- JSON output and its
onlyfield are correct. --failcounts only selected changes: exit 1 on a match, exit 0 on none.- The scoped text report when nothing matches is correct.
net/http,,and" "are rejected with exit 2.- The README and roadmap match this behavior.
- The tests would fail without the change, and they cover flags placed after the directory.
GillMacedo
left a comment
There was a problem hiding this comment.
The change requested in the earlier review is still open: head is still b287567, and an explicitly empty --only value is ignored instead of rejected.
To reproduce, from the repo root:
go run ./plugin/scripts/godebugdiff.go --to 1.24 --only "" --fail examples/shop
go run ./plugin/scripts/godebugdiff.go --to 1.24 --only= --json examples/shop
Both print the full unfiltered result: 15 changes, with no Packages: line in text and no only field in JSON. With --fail, the first exits 1 because of packages the caller may have meant to exclude. By contrast, --only " " is rejected with --only requires comma-separated non-empty package names and exit status 2. In a script, --only "$PKGS" --fail with an empty variable silently drops the filter. This also contradicts the PR's statement that empty entries are rejected.
To fix: reject an explicitly set empty value with the same error and exit code 2, for example by using fs.Visit to detect whether only was set. Keep the unfiltered behavior when the flag is omitted. Add a case to TestOnlyPackages or another test that goes through run, because filterPackages can't tell an omitted flag from an empty value.
Checked and working at this commit:
scripts/pr-checkexited 0, andmake testpasses.- The risky lines it flagged are README text and test data. The diff adds no network access or file writes.
--only "net/http, crypto/tls" --failonexamples/shopselects exactly 5 of the 15 changes, shows thePackages:line, narrows the pin block and exits 1.- The README and roadmap describe the filter, and issue #5's flag, filter, unit test and README line are all present.
GillMacedo
left a comment
There was a problem hiding this comment.
One change needed: a mistyped --only package reads as all clear, so a CI gate using --fail passes without checking anything.
To reproduce, from the repo root:
go run ./plugin/scripts/godebugdiff.go --to 1.24 --only net/htp --fail examples/shop
go run ./plugin/scripts/godebugdiff.go examples/shop --to 1.24 --only net/http/ --json --fail
Both exit 0. The first prints No GODEBUG defaults change in the selected packages. and the second prints "changes": [], although 4 settings change in net/http on this module and --only net/http --fail exits 1. A correct name with no changes (--to 1.21 --only net/http --fail) gives the same message and exit code, so the caller can't tell a typo from a real result.
To fix: reject any --only name that isn't a package in the toolchain's GODEBUG table, using the existing loadTable, with exit code 2 and an error naming the package. Keep a valid package with no changes as exit 0, and don't fail on valid names if the table can't be read. TestOnlyPackages uses --only=example.com/not-imported as its no-match case, so switch it to a real package with no change in the range, such as --to 1.21 --only net/http, and add a case for an unknown name that goes through run.
Also fix while you're there: an explicitly empty value is still ignored. --only "" --fail and --only= print the full 15-change report with no filter, while --only " " exits 2. This wouldn't hold up the PR alone, because it reports more rather than less. Reject it with the same error, using fs.Visit to tell it apart from an omitted flag.
Checked and working at this commit:
scripts/pr-checkexited 0 andmake testpasses. The flagged lines are README text and test data, and the diff adds no network access or file writes.--only "net/http, crypto/tls" --failonexamples/shopselects exactly 5 of the 15 changes, shows thePackages:line, narrows the pin block and exits 1.--onlyworks after the directory, and JSON output includes theonlyfield.--only "net/http,,"exits with status 2 and the expected error.- The README, roadmap and PR title describe the filter, and issue #5's flag, filter, unit test and README line are all present.
Issue
Closes #5
What changed
Adds
--only net/http,crypto/tlsto select GODEBUG changes by exact package name. It works before or after the module directory, supports JSON and text reports, and limits--failto the selected changes. Whitespace and duplicate names are handled; empty list entries are rejected. Text reports explicitly identify the selected packages, including when no selected settings change.README usage and the roadmap are updated. The default unfiltered behavior is unchanged.
Validation
--onlywas unknown.make testpassed: formatting, vet, and 21 unit/integration tests using Go 1.27.3, including the existing real-binary comparisons.make lintpassed using Claude CLI 2.1.316 in an isolated npm execution environment. The installed older CLI lacks--strict; no global CLI was upgraded. The current validator emits the existing non-blocking missing-icon advisory.git diff --checkpassed.Checklist
make testpasses