Show full nodeID and executionID with --wide flag #4783
Conversation
WalkthroughThis pull request introduces modifications across several CLI-related files, focusing on improving the display and handling of identifiers, columns, and job history information. The changes primarily involve updating column configurations, renaming certain columns, and refining how IDs and version information are presented in the command-line interface. The modifications aim to enhance the clarity and consistency of output across different command contexts. Changes
Possibly related PRs
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cmd/cli/job/executions.go(1 hunks)cmd/cli/job/history.go(1 hunks)cmd/cli/node/columns.go(1 hunks)cmd/util/cols/history.go(1 hunks)
🔇 Additional comments (4)
cmd/util/cols/history.go (1)
30-39: Verify the impact of showing full execution IDs
The changes to show full execution IDs align with the PR objectives, but we should verify:
- That the
idgen.ShortIDLengthWithPrefixconstant is appropriate for the column width - That the UI remains readable with full IDs
Additionally, consider adding a comment explaining that this configuration supports the --wide flag behavior.
✅ Verification successful
Based on the search results, I can now generate the final response as the findings provide comprehensive information about the usage of both ShortIDLengthWithPrefix and ShortUUID across the codebase.
The ID length and shortening approach is consistently used across the codebase
The changes to show full execution IDs are well-aligned with existing patterns in the codebase:
ShortIDLengthWithPrefixis consistently used as theWidthMaxfor ID columns across multiple commands (job list, executions, node state)ShortUUIDis the standard width enforcer for UUID-based IDs throughout the CLI- The implementation in
pkg/util/idgen/short_id.goshows this is a well-tested and documented functionality
The suggestion to add a comment about --wide flag behavior is a valid nitpick but not a functional concern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check the length constant and its usage
rg "ShortIDLengthWithPrefix" --type go -A 2
# Check other places where ShortUUID is used
rg "ShortUUID" --type go -A 2
Length of output: 4626
cmd/cli/job/history.go (1)
Line range hint 91-95: LGTM: Column replacement improves time display clarity
The switch from HistoryTime to HistoryDateTime provides more detailed timestamp information, which is a good improvement.
cmd/cli/node/columns.go (1)
Line range hint 52-67: LGTM: Enhanced version information improves node details
The addition of architecture and OS columns provides valuable system information that can help with debugging and deployment decisions.
cmd/cli/job/executions.go (1)
Line range hint 41-54: Verify --wide flag setup
Before implementing the suggested changes, we need to ensure the --wide flag is properly set up and accessible to the column configurations.
✅ Verification successful
The --wide flag is properly set up and ready to use
The verification shows that the --wide flag is correctly implemented and accessible:
- It's defined in
cmd/util/flags/cliflags/output.goas a boolean flag ExecutionOptionsembedsoutput.OutputOptionswhich contains theWidefield- The output package properly handles the wide flag in table rendering by removing width constraints when the flag is enabled
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if and where the --wide flag is defined
# Search for --wide flag definition in the codebase
rg -A 5 'AddFlagSet.*wide|Flag\(.*wide'
# Search for any usage of the wide flag in the output package
rg -A 5 'wide' pkg/util/output/
Length of output: 130
Script:
#!/bin/bash
# Let's try different approaches to locate the wide flag and output package
# Find the output package location
fd -t d output
# Search for wide flag in the entire codebase
rg -i --type go "wide.*flag|flag.*wide"
# Search for output package imports
rg -i --type go "^import.*output"
Length of output: 197
Script:
#!/bin/bash
# Let's check the output flag implementation and its usage
# Check the output flag implementation
cat cmd/util/flags/cliflags/output.go
# Check if ExecutionOptions embeds or uses the output format
rg -A 5 "type ExecutionOptions" cmd/cli/job/executions.go
# Check the output package in cmd/util/output
ls -la cmd/util/output/
cat cmd/util/output/*.go
Length of output: 10566
| ColumnConfig: table.ColumnConfig{ | ||
| Name: "id", | ||
| WidthMax: idgen.ShortIDLengthWithPrefix, | ||
| WidthMaxEnforcer: func(col string, maxLen int) string { return idgen.ShortNodeID(col) }}, | ||
| Value: func(node *models.NodeState) string { return node.Info.ID() }, |
There was a problem hiding this comment.
💡 Codebase verification
Based on the implementation of ShortNodeID, I can now generate the final response:
ShortNodeID conflicts with full ID display objective
The current implementation of ShortNodeID actively shortens IDs:
- For UUIDs: returns only the first segment (e.g., "78faf114" from a full UUID)
- For libp2p IDs: truncates to first 8 characters
- This contradicts the objective of showing full node IDs in the table
This creates an inconsistency where:
Valuefunction returns the full ID:node.Info.ID()- But
WidthMaxEnforcerforcibly shortens it usingShortNodeID
🔗 Analysis chain
Verify the node ID display behavior
The changes to show full node IDs align with the PR objectives. However:
- Ensure that
idgen.ShortNodeIDin WidthMaxEnforcer is consistent with the full ID display objective - Verify that the UI remains readable with full node IDs
Consider adding a comment explaining the relationship between these settings and the --wide flag behavior.
Let me analyze the implementation of ShortNodeID to understand if it conflicts with the full ID display objective. Let's check the actual implementation and the ShortUUID function it uses.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check the implementation of ShortNodeID
ast-grep --pattern 'func ShortNodeID($id) { $$$ }'
# Check other usages of ShortNodeID
rg "ShortNodeID" --type go -A 2
Length of output: 2214
Script:
#!/bin/bash
# Check the full implementation of ShortNodeID and ShortUUID
cat pkg/util/idgen/short_id.go
# Check if there's a --wide flag implementation that might affect this behavior
rg --type go "wide.*flag" -A 2
Length of output: 2111
| ColumnConfig: table.ColumnConfig{ | ||
| Name: "Node ID", | ||
| WidthMax: idgen.ShortIDLengthWithPrefix, | ||
| WidthMaxEnforcer: func(col string, maxLen int) string { return idgen.ShortUUID(col) }}, | ||
| Value: func(e *models.Execution) string { return e.NodeID }, |
There was a problem hiding this comment.
Critical: Implementation doesn't respect --wide flag for NodeID
Similar to the ID column, the NodeID column's implementation doesn't fulfill the PR objective of showing full IDs with the --wide flag.
Apply similar changes as suggested for the ID column:
executionColumnNodeID = output.TableColumn[*models.Execution]{
ColumnConfig: table.ColumnConfig{
Name: "Node ID",
- WidthMax: idgen.ShortIDLengthWithPrefix,
- WidthMaxEnforcer: func(col string, maxLen int) string { return idgen.ShortUUID(col) }},
+ WidthMax: func(cmd *cobra.Command) int {
+ if cmd.Flags().Changed("wide") {
+ return 0 // No max width for full ID
+ }
+ return idgen.ShortIDLengthWithPrefix
+ },
+ WidthMaxEnforcer: func(col string, maxLen int) string {
+ if maxLen == 0 {
+ return col // Return full ID
+ }
+ return idgen.ShortUUID(col)
+ }},
Value: func(e *models.Execution) string { return e.NodeID },📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ColumnConfig: table.ColumnConfig{ | |
| Name: "Node ID", | |
| WidthMax: idgen.ShortIDLengthWithPrefix, | |
| WidthMaxEnforcer: func(col string, maxLen int) string { return idgen.ShortUUID(col) }}, | |
| Value: func(e *models.Execution) string { return e.NodeID }, | |
| ColumnConfig: table.ColumnConfig{ | |
| Name: "Node ID", | |
| WidthMax: func(cmd *cobra.Command) int { | |
| if cmd.Flags().Changed("wide") { | |
| return 0 // No max width for full ID | |
| } | |
| return idgen.ShortIDLengthWithPrefix | |
| }, | |
| WidthMaxEnforcer: func(col string, maxLen int) string { | |
| if maxLen == 0 { | |
| return col // Return full ID | |
| } | |
| return idgen.ShortUUID(col) | |
| }}, | |
| Value: func(e *models.Execution) string { return e.NodeID }, |
| ColumnConfig: table.ColumnConfig{ | ||
| Name: "ID", | ||
| WidthMax: idgen.ShortIDLengthWithPrefix, | ||
| WidthMaxEnforcer: func(col string, maxLen int) string { return idgen.ShortUUID(col) }}, | ||
| Value: func(e *models.Execution) string { return e.ID }, |
There was a problem hiding this comment.
Critical: Implementation doesn't respect --wide flag for showing full IDs
The current implementation doesn't fulfill the PR objective of showing full IDs with the --wide flag. The Value function returns the full ID but the WidthMaxEnforcer always shortens it, regardless of the --wide flag state.
Consider this implementation:
executionColumnID = output.TableColumn[*models.Execution]{
ColumnConfig: table.ColumnConfig{
Name: "ID",
- WidthMax: idgen.ShortIDLengthWithPrefix,
- WidthMaxEnforcer: func(col string, maxLen int) string { return idgen.ShortUUID(col) }},
+ WidthMax: func(cmd *cobra.Command) int {
+ if cmd.Flags().Changed("wide") {
+ return 0 // No max width for full ID
+ }
+ return idgen.ShortIDLengthWithPrefix
+ },
+ WidthMaxEnforcer: func(col string, maxLen int) string {
+ if maxLen == 0 {
+ return col // Return full ID
+ }
+ return idgen.ShortUUID(col)
+ }},
Value: func(e *models.Execution) string { return e.ID },Committable suggestion skipped: line range outside the PR's diff.
wdbaruni
deleted the
eng-160-show-full-nodeid-and-executionid-with-wide-flag
branch
Summary by CodeRabbit
New Features
Bug Fixes
Chores
HistoryTimecolumn for cleaner output.