fix(filters): --filter was accepted, documented, and ignored on 20 commands - #268
Draft
Denis-hamon wants to merge 1 commit into
Draft
fix(filters): --filter was accepted, documented, and ignored on 20 commands#268Denis-hamon wants to merge 1 commit into
Denis-hamon wants to merge 1 commit into
Conversation
…mmands
withFilterFlag only binds the flag to flags.GenericFilters. The filtering itself
happens inside ManageListRequest and ManageListRequestNoExpand, so a command
that assembles its own rows and calls display.RenderTable gets the flag, its
documentation page, its shell completion — and no filtering. Twenty commands
were in that state, across cloud, vps and webhosting.
Eighteen of them list rows and now filter them, through one helper rather than
the same three lines copied eighteen times. Two do not list anything:
`cloud loadbalancer log list-kinds` answers a list of plain strings, which has
no properties to select on, and `cloud storage object quota get` reads the quota
of one region and renders one object. There the honest correction is to drop the
flag, not to apply it — applying it would only have made the lie harder to see.
Two of the eighteen hand their list to a template inside a wrapper object, so
they filter before the wrapping: once the list is inside an object there are no
rows left to select from. That is what FilteredRows is for.
Five tests, each asserting the row the filter must REMOVE. Asserting the kept row
is present would pass just as well with no filtering at all, which is how these
twenty went unnoticed. Three sabotages, three red.
Two things the instruments got wrong on the way, both caught by controls:
- The first census said 27. Seven of those were correct: they delegate to a
package-local helper — listNetworksByVisibility, listLoadbalancingResources —
which does filter, and the analyser did not follow one level of indirection.
Verified by reading both helpers, then by a positive control that breaks one
and watches its two callers reappear.
- The flag-removal test first looked the flag up with Flags().Lookup("filter").
withFilterFlag registers on PersistentFlags, so that answers nil whether the
flag is there or not: the assertion passed with the flag put back. It runs the
command now, because what matters is what cobra accepts on the command line.
Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
withFilterFlagonly binds the flag toflags.GenericFilters. The filtering itself happens insidecommon.ManageListRequestandcommon.ManageListRequestNoExpand, so a command that assembles its own rows and callsdisplay.RenderTablegets the flag, its documentation page, its shell completion — and no filtering at all.Twenty commands were in that state, across
cloud,vpsandwebhosting.What changes
Eighteen list rows and now filter them, through one helper rather than the same three lines copied eighteen times.
Two do not list anything, and lose the flag instead.
cloud loadbalancer log list-kindsanswers a list of plain strings, which has no properties to select on;cloud storage object quota getreads the quota of one region and renders one object. Applying--filterthere would only have made the lie harder to see.Two of the eighteen filter before wrapping. They hand their list to a template inside a wrapper object, and once the list is inside an object there are no rows left to select from. That is what
common.FilteredRowsis for.Verification
Five tests, each asserting the row the filter must remove. Asserting that the kept row is present would pass just as well with no filtering at all — which is how these twenty went unnoticed.
Three sabotages, three red: the helper made inert while still compiling, the flag put back on one of the two object commands, and
display.RenderTablerestored on one of the eighteen.Gate:
go build,go vet,make wasm,go test ./...,make doc— all green, doc pages regenerated for the two flag removals.Two things the instruments got wrong, and how they were caught
Both by a control, not by a reading — recorded here because the numbers in this PR depend on it.
listNetworksByVisibility,listLoadbalancingResources— which does filter, and the analyser did not follow one level of indirection. Verified by reading both helpers, then by a positive control that breaks one and watches its two callers reappear in the census.Flags().Lookup("filter").withFilterFlagregisters onPersistentFlags, so that answersnilwhether the flag is there or not: the assertion passed with the flag put back. It runs the command now, because what matters is what cobra accepts on the command line.Scope
Based on
main, independent of the BareMetal series. The same defect existed on twenty-one commands of that series and is fixed there separately; the helper added here is the same one, with the same name and body, so if both land the merge is trivial.🤖 Generated with Claude Code