OCM-1520 | feat: Add --hide-empty-columns flag to rosa list commands #2983
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: olucasfreitas The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #2983 +/- ##
==========================================
+ Coverage 27.37% 27.76% +0.39%
==========================================
Files 306 317 +11
Lines 34350 35291 +941
==========================================
+ Hits 9403 9799 +396
- Misses 24301 24832 +531
- Partials 646 660 +14 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
olucasfreitas
force-pushed
the
OCM-1520
branch
from
dd91dc0 to
cbf08f0
Compare
|
@hunterkepley addressed all the comments in the new commit |
olucasfreitas
force-pushed
the
OCM-1520
branch
2 times, most recently
from
ef825a0 to
eb6702d
Compare
olucasfreitas
force-pushed
the
OCM-1520
branch
2 times, most recently
from
2f270f0 to
bdf62a6
Compare
|
@hunterkepley new comments addressed on the new commit |
|
Here's a Go Playground showing the solution I came up with for a list of separators: https://go.dev/play/p/2qUevkuSUUz We can pass the table data- including the new string separator list, allowing the custom format we require for some commands -into this and it will build a usable output to pass into a tabwriter, with less LoC |
olucasfreitas
force-pushed
the
OCM-1520
branch
from
bdf62a6 to
cec4bd1
Compare
|
/retest |
|
/test coverage |
pkg/output/table_filter.go
Outdated
| if len(row) == 0 { | ||
| continue | ||
| } |
There was a problem hiding this comment.
I'm curious, is there a case where we would expect the length of a row to be 0 in the middle of a table?
There was a problem hiding this comment.
for what i understood here, empty rows can occur in real-world data
There was a problem hiding this comment.
Could you provide an example?
There was a problem hiding this comment.
Data Import with Blank Lines
// CSV parsing or API responses might produce:
tableData := [][]string{
{"ID", "Name", "Status"},
{"1", "Alice", "Active"},
{}, // Blank line in source data
{"2", "Bob", "Pending"},
}Important Distinction
// These are different:
emptyRow := []string{} // Length 0 - gets skipped ✓
blankRow := []string{"", "", ""} // Length 3 - displays with spacingThere was a problem hiding this comment.
Hey Lucas, thanks for explaining the logic here;
I was more-so asking for a 'real world' example, such as one coming from the ROSA CLI. My thoughts are, that this may not be possible for our tables in the ROSA CLI, just by design. We add data generally when data exists, so there should be at least one value in each row (for loops over resource lists add data by nature causes this)
BUT- im asking for examples because I want to see if I might be wrong here, and if there's a list command which can produce empty rows
There was a problem hiding this comment.
For example, if we list access requests - we use the API to fetch a list of the cluster's access requests. Then we loop through those
This pattern is pretty common across all list commands, where we fetch data before listing, since that data is generally not available locally on the user's machine
Hopefully that makes senes, LMK if you have questions
There was a problem hiding this comment.
Oh, now I got it, I'm gonna run through some commands a send you the data here
There was a problem hiding this comment.
So, I ran through the commands, and I stand corrected, no cases in where this can happen showed up for me, thanks for the explanation here, I addressed the changes on this commit e397f65
pkg/output/table_filter.go
Outdated
| separators := make([]string, maxCols) | ||
| for i := range separators { | ||
| separators[i] = separator | ||
| } | ||
|
|
||
| BuildTableWithSeparators(writer, separators, tableData) |
There was a problem hiding this comment.
In this comment I talked about having support for separators which have different separators at different parts of the table. For example, val1\tval2\t\tval3
It seems like you have support for it now, but did not actually support it, instead wrapping it in a function which makes a copy of a single separator that was passed in
If we could make that functional, and get the original separators back in the command which have unique separator values per column, that would be great
There was a problem hiding this comment.
For example, list machinepools
There was a problem hiding this comment.
Improved this on the last commit
There was a problem hiding this comment.
@olucasfreitas im afraid this was virtually unchanged from the last commit
There was a problem hiding this comment.
Worked on this on this commit 3f5390f
|
@olucasfreitas What is |
|
@hunterkepley the mock mode was something I added only for testing, to activate the mock responses on the command |
@olucasfreitas You made an entire mock mode only for testing this feature? |
olucasfreitas
force-pushed
the
OCM-1520
branch
from
cec4bd1 to
7de32b5
Compare
|
@hunterkepley was actually pretty simple, not an ENTIRE mock mode as you maybe thought |
|
/retest |
pkg/machinepool/machinepool_test.go
Outdated
| headers, tableData := getNodePoolsData(cluster.NodePools().Slice()) | ||
| out := formatTableString(headers, tableData) | ||
|
|
||
| expectedOutput := fmt.Sprintf("ID\tAUTOSCALING\tREPLICAS\tINSTANCE TYPE\tLABELS\tTAINTS\tAVAILABILITY ZONE\tSUBNET\tDISK SIZE\tVERSION\tAUTOREPAIR\n"+ |
There was a problem hiding this comment.
You are still not taking into account different separators within a single table
There was a problem hiding this comment.
Worked on this on this commit 3f5390f
pkg/output/table_filter.go
Outdated
| return true | ||
| } | ||
|
|
||
| type ColumnFilterFunc func(columnIdx int) bool |
There was a problem hiding this comment.
Why was this introduced?
There was a problem hiding this comment.
This was created to support the new FilterColumns function, which needed a way to let callers specify custom logic for which columns to keep or remove. I did this for the flexibility, reusability and composability
There was a problem hiding this comment.
Cool yeah that makes sense,
I did have a question about this:
// Use FilterColumns with a predicate that keeps non-empty columns
newHeaders, newTableData := FilterColumns(headers, tableData, func(columnIdx int) bool {
return !CheckIfColumnIsEmpty(columnIdx, tableData)
})I see that this is used twice, to filter columns based on !CheckIfColumnIsEmpty
I think this is greatly useful if we had different types of filtering to be done, but in our case here I see we have only CheckIfColumnIsEmpty
My thoughts are, that we can get rid of the type here, and instead of passing a function into FilterColumns, we can directly call CheckIfColumnIsEmpty. WDYT?
OH- another note;
The current design has it so tableData is passed into both FilterColumns and the function being passed in.
// Use FilterColumns with a predicate that keeps non-empty columns
newHeaders, newTableData := FilterColumns(headers, tableData, func(columnIdx int) bool {
return !CheckIfColumnIsEmpty(columnIdx, tableData)
})Looking at this again, we can see that. Sadly, this means this data (no matter how large or small) is tripled in memory every time this is called (once for the first duplication to pass into the function, and a second one for the CheckIfColumnIsEmpty func
There was a problem hiding this comment.
While the memory part may not seem like a big deal at first, we have to think about the fact that some tables can be very long. And we have the ability to control the pagination with the CLI commands usually
There was a problem hiding this comment.
My thoughts are, that we can get rid of the type here, and instead of passing a function into FilterColumns, we can directly call CheckIfColumnIsEmpty. WDYT?
I do agree with this, makes this much simpler to understand and the implementation cleaner too
While the memory part may not seem like a big deal at first, we have to think about the fact that some tables can be very long. And we have the ability to control the pagination with the CLI commands usually
Did not think of this, thanks for the explanation, will take a look into to fixing it on a new commit
There was a problem hiding this comment.
Hopefully addressed on 75d64ee
pkg/output/table_filter.go
Outdated
| separators := make([]string, maxCols) | ||
| for i := range separators { | ||
| separators[i] = separator | ||
| } | ||
|
|
||
| BuildTableWithSeparators(writer, separators, tableData) |
There was a problem hiding this comment.
@olucasfreitas im afraid this was virtually unchanged from the last commit
pkg/machinepool/machinepool.go
Outdated
| // Get headers and data | ||
| headers, tableData := getMachinePoolsData(r, machinePools, args) | ||
| if isHypershift { | ||
| finalStringToOutput = getNodePoolsString(nodePools) | ||
| headers, tableData = getNodePoolsData(nodePools) | ||
| } |
There was a problem hiding this comment.
In the original functions (getMachinePoolsString and getNodePoolsString), we return a string such as
test1\ttest2\ttest3\nval1\tval2\tval3\n
Couldn't we convert this into a list and not change the original functions?
There was a problem hiding this comment.
Worked on this on this commit 3f5390f
olucasfreitas
force-pushed
the
OCM-1520
branch
from
7de32b5 to
3f5390f
Compare
olucasfreitas
force-pushed
the
OCM-1520
branch
from
3f5390f to
e4f2322
Compare
|
Oh it looks like it may have squashed- be sure to check this page for replies on comments, the old ones won't show up on the |
|
/retest |
1 similar comment
|
/retest |
| return headers, tableData | ||
| } | ||
|
|
||
| // Write header |
There was a problem hiding this comment.
Hey I think we need to include unique separators for the nodepool output- you can see here that the original separator list has a mix of \t\t and \t
It seems support was added for it in the last commit but it was not implemented in the actual command yet (still using a single separator \t for this command for example)
There was a problem hiding this comment.
@hunterkepley adress this on the new commit, please take a look when you can
There was a problem hiding this comment.
@olucasfreitas Hey it looks like this wasn't changed- the BuildTableWithSeparators function is not called in the machinepool command still
There was a problem hiding this comment.
it looks like it's only used in tests
There was a problem hiding this comment.
Sorry, I forgot to push the rest of the changes, please check it again
olucasfreitas
force-pushed
the
OCM-1520
branch
from
bea53d8 to
fc6c0ed
Compare
|
@olucasfreitas: all tests passed! Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
Details
This PR improves the UX when listing resources by introducing a new
--hide-empty-columnsflag that automatically hides columns containing no data across all rows, making the output cleaner and more readable.The flag has been added to the following
rosa listcommands:Note: The following commands do NOT have this flag:
list machinepools- Already has built-in auto-hide empty columns logiclist version,list userroles,list user,list rhRegion,list tuningconfigs,list oidcprovider,list kubeletconfig,list externalauthprovider- These maintain their original behaviorDefault Behavior (All Columns Shown)
Run any supported list command without the flag:
You should see all columns displayed, including empty ones:
Using --hide-empty-columns Flag
Run the same command with the flag:
Empty columns (like TOPOLOGY in this example) should be automatically hidden:
New behavior of the '--all'
The
--allflag shows the 3 additional columns (AZ TYPE, WIN-LI ENABLED, DEDICATED HOST) but still hides empty columns:Example output:
Ticket
Closes [OCM-1520]