Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Show full nodeID and executionID with --wide flag #4783

Merged
merged 1 commit into from
Dec 18, 2024
Merged

Show full nodeID and executionID with --wide flag #4783

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 10 additions & 4 deletions cmd/cli/job/executions.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,12 +106,18 @@ var (
Value: func(e *models.Execution) string { return output.Elapsed(e.GetModifyTime()) },
}
executionColumnID = output.TableColumn[*models.Execution]{
ColumnConfig: table.ColumnConfig{Name: "ID", WidthMax: 10, WidthMaxEnforcer: text.WrapText},
Value: func(e *models.Execution) string { return idgen.ShortUUID(e.ID) },
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 },
Comment on lines +109 to +113
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

}
executionColumnNodeID = output.TableColumn[*models.Execution]{
ColumnConfig: table.ColumnConfig{Name: "Node ID", WidthMax: 10, WidthMaxEnforcer: text.WrapText},
Value: func(e *models.Execution) string { return idgen.ShortNodeID(e.NodeID) },
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 },
Comment on lines +116 to +120
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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 },

}
executionColumnRev = output.TableColumn[*models.Execution]{
ColumnConfig: table.ColumnConfig{Name: "Rev.", WidthMax: 4, WidthMaxEnforcer: text.WrapText},
Expand Down
2 changes: 1 addition & 1 deletion cmd/cli/job/history.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ func NewHistoryCmd() *cobra.Command {
}

var historyColumns = []output.TableColumn[*models.JobHistory]{
cols.HistoryTime,
cols.HistoryDateTime,
cols.HistoryLevel,
cols.HistoryExecID,
cols.HistoryTopic,
Expand Down
7 changes: 5 additions & 2 deletions cmd/cli/node/columns.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,11 @@ import (

var alwaysColumns = []output.TableColumn[*models.NodeState]{
{
ColumnConfig: table.ColumnConfig{Name: "id"},
Value: func(node *models.NodeState) string { return idgen.ShortNodeID(node.Info.ID()) },
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() },
Comment on lines +20 to +24
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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:

  • Value function returns the full ID: node.Info.ID()
  • But WidthMaxEnforcer forcibly shortens it using ShortNodeID
🔗 Analysis chain

Verify the node ID display behavior

The changes to show full node IDs align with the PR objectives. However:

  1. Ensure that idgen.ShortNodeID in WidthMaxEnforcer is consistent with the full ID display objective
  2. 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: "type"},
Expand Down
11 changes: 5 additions & 6 deletions cmd/util/cols/history.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,6 @@ import (
)

var (
HistoryTime = output.TableColumn[*models.JobHistory]{
ColumnConfig: table.ColumnConfig{Name: "Time", WidthMax: len(time.StampMilli), WidthMaxEnforcer: output.ShortenTime},
Value: func(j *models.JobHistory) string { return j.Occurred().Format(time.StampMilli) },
}
HistoryTimeOnly = output.TableColumn[*models.JobHistory]{
ColumnConfig: table.ColumnConfig{Name: "Time", WidthMax: len(TimeOnlyMilli), WidthMaxEnforcer: text.Trim},
Value: func(j *models.JobHistory) string { return j.Occurred().Format(TimeOnlyMilli) },
Expand All @@ -31,12 +27,15 @@ var (
Value: func(jwi *models.JobHistory) string { return jwi.Type.String() },
}
HistoryExecID = output.TableColumn[*models.JobHistory]{
ColumnConfig: table.ColumnConfig{Name: "Exec. ID", WidthMax: 10, WidthMaxEnforcer: text.WrapText},
ColumnConfig: table.ColumnConfig{
Name: "Exec. ID",
WidthMax: idgen.ShortIDLengthWithPrefix,
WidthMaxEnforcer: func(col string, maxLen int) string { return idgen.ShortUUID(col) }},
Value: func(j *models.JobHistory) string {
if j.ExecutionID == "" {
return ""
}
return idgen.ShortUUID(j.ExecutionID)
return j.ExecutionID
},
}
HistoryTopic = output.TableColumn[*models.JobHistory]{
Expand Down
Loading