consolidate json and utc mode handling #1885
Conversation
📝 WalkthroughWalkthroughThe PR centralizes Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)level=warning msg="[linters_context] running gomodguard failed: unable to read module file go.mod: current working directory must have a go.mod file: if you are not using go modules it is suggested to disable this linter" Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@src/cmd/cli/command/commands.go`:
- Around line 168-169: The root-level flag
RootCmd.PersistentFlags().BoolVarP(&global.Json, "json", "", false, "show output
in JSON format") conflicts with the command-specific
whoamiCmd.PersistentFlags().Bool("json", pkg.GetenvBool("DEFANG_JSON"), "print
output in JSON format"); remove the whoamiCmd-specific JSON flag and rely on the
global.Json persistent flag instead so all commands share the same default and
behavior (delete the whoamiCmd.PersistentFlags().Bool(...) line and update any
whoami command code that reads its local flag to reference global.Json).
In `@src/cmd/cli/command/workspace.go`:
- Around line 39-41: The comment notes inconsistent flag scoping: workspaceCmd
and workspaceListCmd use local Flags() for --json while the whoami command uses
PersistentFlags(), which can confuse readers; standardize the pattern by
changing the subcommands to define the --json flag the same way (either all use
PersistentFlags() or all use Flags()) so SetJSONMode (invoked in
PersistentPreRunE) and term.Table behavior remain consistent; update the flag
registration for workspaceCmd and workspaceListCmd (or alternatively adjust
whoami) to match the chosen approach and run the flag parsing path through
SetJSONMode to verify DEFANG_JSON is set as expected.
In `@src/pkg/cli/tail_test.go`:
- Line 251: SetUTCMode(true) is set in the test without restoring it, causing
global test state leakage; modify the test around the SetUTCMode(true) call to
restore the original state by registering a cleanup that calls SetUTCMode(false)
(or using defer if appropriate) so UTC mode is reset after the test finishes;
locate the call to SetUTCMode in tail_test.go and add a t.Cleanup(func(){
SetUTCMode(false) }) (or equivalent) immediately after it.
In `@src/pkg/term/table.go`:
- Around line 32-38: The JSON keys are using struct field names (attr) instead
of honoring json tags; update the loop that builds filtered (reference
variables: filtered, attributes, item, field) to look up the StructField tag via
reflect.Type.FieldByName(attr), read sf.Tag.Get("json"), split on ',' and use
the tag name if non-empty and not "-", otherwise fall back to attr; assign
filtered[i][jsonName] = field.Interface(); also add the "strings" import for
splitting the tag.
🧹 Nitpick comments (4)
src/pkg/cli/deploymentsList.go (1)
61-65: Consider guarding invalid timestamps to avoid “0001-01-01” output.If the API ever returns an invalid/unset timestamp, formatting the zero time is misleading. Using
timeutils.AsTimeand emitting an empty string (or “N/A”) keeps the table cleaner.♻️ Suggested update
import ( "context" "sort" "strings" "time" "github.com/DefangLabs/defang/src/pkg/cli/client" "github.com/DefangLabs/defang/src/pkg/term" + "github.com/DefangLabs/defang/src/pkg/timeutils" defangv1 "github.com/DefangLabs/defang/src/protos/io/defang/v1" ) @@ for i, d := range response.Deployments { - deployedAt := d.Timestamp.AsTime().Local().Format(time.RFC3339) + ts := timeutils.AsTime(d.Timestamp, time.Time{}) + deployedAt := "" + if !ts.IsZero() { + deployedAt = ts.Local().Format(time.RFC3339) + } deployments[i] = DeploymentLineItem{ AccountId: d.ProviderAccountId, DeployedAt: deployedAt,src/pkg/term/table.go (1)
18-47: Consider extracting the JSON mode check for reusability and testability.The hardcoded
os.Getenv("DEFANG_JSON") == "1"check could be extracted to a helper function (perhaps in theclipackage alongsideSetJSONMode) to ensure consistency and make testing easier.src/pkg/cli/tail_test.go (1)
384-384: Inconsistenttime.Localmodification bypassesSetUTCModeAPI.
TestPrintHandlerdirectly modifiestime.Localinstead of using the newSetUTCModeAPI. This is inconsistent with the centralized approach and doesn't restore the original value after the test.♻️ Proposed fix to use SetUTCMode and add cleanup
loc, err := time.LoadLocation("America/Vancouver") if err != nil { t.Fatalf("Failed to load location: %v", err) } - time.Local = loc + originalLocal := time.Local + time.Local = loc + t.Cleanup(func() { time.Local = originalLocal }) for _, tc := range testdata {Note: Since this test needs a specific timezone (not UTC), it can't use
SetUTCMode. Adding cleanup ensures state isn't leaked.src/cmd/cli/command/commands.go (1)
365-372: Consider using bound global variables directly.Since the flags are already bound to
global.Jsonandglobal.UtcviaBoolVarP(lines 168-169), you could simplify by using the global variables directly instead ofcmd.Flags().GetBool():cli.SetUTCMode(global.Utc) cli.SetJSONMode(global.Json) if global.Json { global.Verbose = true }This would also avoid shadowing Go's built-in
jsonidentifier with the local variable. However, this depends on resolving the flag conflict withwhoamiCmdmentioned above.
| RootCmd.PersistentFlags().BoolVarP(&global.Json, "json", "", false, "show output in JSON format") | ||
| RootCmd.PersistentFlags().BoolVarP(&global.Utc, "utc", "", false, "show timestamps in UTC timezone") |
There was a problem hiding this comment.
Potential flag conflict with whoamiCmd.
The new root-level --json flag conflicts with the existing flag at line 216:
whoamiCmd.PersistentFlags().Bool("json", pkg.GetenvBool("DEFANG_JSON"), "print output in JSON format")The whoamiCmd flag shadows the root's persistent flag, causing inconsistent behavior: whoami --json uses the environment variable default, while other commands default to false. Consider removing the whoamiCmd-specific flag to rely on the root-level flag instead.
🤖 Prompt for AI Agents
In `@src/cmd/cli/command/commands.go` around lines 168 - 169, The root-level flag
RootCmd.PersistentFlags().BoolVarP(&global.Json, "json", "", false, "show output
in JSON format") conflicts with the command-specific
whoamiCmd.PersistentFlags().Bool("json", pkg.GetenvBool("DEFANG_JSON"), "print
output in JSON format"); remove the whoamiCmd-specific JSON flag and rely on the
global.Json persistent flag instead so all commands share the same default and
behavior (delete the whoamiCmd.PersistentFlags().Bool(...) line and update any
whoami command code that reads its local flag to reference global.Json).
| if verbose && !jsonMode { | ||
| headers = []string{"Name", "ID", "Current"} | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's see how SetJSONMode is implemented and used
rg -n -A5 'SetJSONMode' src/cmd/cli/command/Repository: DefangLabs/defang
Length of output: 411
🏁 Script executed:
#!/bin/bash
# Now let's look at the workspace.go file in full context, especially around the flagged lines
wc -l src/cmd/cli/command/workspace.goRepository: DefangLabs/defang
Length of output: 96
🏁 Script executed:
#!/bin/bash
# Check if there's a root command setup that handles JSON mode
rg -n 'jsonMode|--json' src/cmd/cli/command/workspace.go | head -30Repository: DefangLabs/defang
Length of output: 137
🏁 Script executed:
#!/bin/bash
# Let's look at the full SetJSONMode implementation in cli.go or util.go
rg -n -B2 -A10 'func SetJSONMode' src/cmd/cli/Repository: DefangLabs/defang
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Check the workspace.go file in full to see the context around lines 15, 39, 43, 65-66
cat -n src/cmd/cli/command/workspace.goRepository: DefangLabs/defang
Length of output: 2346
🏁 Script executed:
#!/bin/bash
# Find where the local --json flag is registered in workspace.go
rg -n -B2 -A2 'flags.*json|json.*flags' src/cmd/cli/command/workspace.goRepository: DefangLabs/defang
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Check if the root command passes flags to subcommands
rg -n 'InheritedFlags|LocalFlags|Flags' src/cmd/cli/command/commands.go | grep -i jsonRepository: DefangLabs/defang
Length of output: 319
🏁 Script executed:
#!/bin/bash
# Let's find the term.Table implementation to see how it uses JSON mode
rg -n -B3 -A15 'func.*Table' src/pkg/term/ | head -60Repository: DefangLabs/defang
Length of output: 1268
🏁 Script executed:
#!/bin/bash
# Check what SetJSONMode does
find . -name "*.go" -path "*/pkg/cli/*" -type f | xargs rg -l 'SetJSONMode'Repository: DefangLabs/defang
Length of output: 82
🏁 Script executed:
#!/bin/bash
# Look at the actual SetJSONMode implementation
rg -n -B2 -A10 'func.*SetJSONMode' src/pkg/Repository: DefangLabs/defang
Length of output: 556
🏁 Script executed:
#!/bin/bash
# Check how the root --json flag (global.Json) connects to term.Table or JSON output
rg -n 'global\.Json' src/cmd/cli/ | head -20Repository: DefangLabs/defang
Length of output: 195
🏁 Script executed:
#!/bin/bash
# Let's trace the command execution flow - check where SetJSONMode is called relative to command execution
rg -n -B5 -A8 'SetJSONMode' src/cmd/cli/command/commands.goRepository: DefangLabs/defang
Length of output: 449
🏁 Script executed:
#!/bin/bash
# Check if there's a PreRun or PreRunE that calls SetJSONMode
rg -n 'PreRun|PersistentPreRun' src/cmd/cli/command/commands.go | head -20Repository: DefangLabs/defang
Length of output: 138
🏁 Script executed:
#!/bin/bash
# Let's see the full context around line 369 where SetJSONMode is called
sed -n '360,380p' src/cmd/cli/command/commands.goRepository: DefangLabs/defang
Length of output: 577
🏁 Script executed:
#!/bin/bash
# Let's verify the exact execution order - check how Cobra processes PersistentFlags and local flags
# See what flags are inherited vs local in the workspace command
rg -n 'workspace.*Flags\|json.*Flags' src/cmd/cli/command/workspace.goRepository: DefangLabs/defang
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Let's confirm: when both root PersistentFlag and local Flag exist with same name,
# which takes precedence? Check if there are other commands with similar pattern
rg -n 'PersistentFlags.*json' src/cmd/cli/command/ -A2 | head -30Repository: DefangLabs/defang
Length of output: 617
🏁 Script executed:
#!/bin/bash
# Critical: Check if there are any other workspace-related commands or if workspace is the only place with local --json
rg -n 'workspaceCmd\|workspaceListCmd' src/cmd/cli/command/Repository: DefangLabs/defang
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Let me check the actual command registration and how workspace commands are added to root
rg -n 'AddCommand.*workspace\|workspaceCmd' src/cmd/cli/command/Repository: DefangLabs/defang
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Verify the init() function that registers the workspace commands
sed -n '46,68p' src/cmd/cli/command/workspace.goRepository: DefangLabs/defang
Length of output: 786
🏁 Script executed:
#!/bin/bash
# Check if there are any other commands with both root PersistentFlag AND local Flag for same option
rg -n 'Flags.*Bool.*json|PersistentFlags.*json' src/cmd/cli/command/ | grep -v commands.goRepository: DefangLabs/defang
Length of output: 666
🏁 Script executed:
#!/bin/bash
# Check if workspaceCmd is registered with RootCmd
rg -n 'RootCmd.AddCommand' src/cmd/cli/command/ | grep -i workspaceRepository: DefangLabs/defang
Length of output: 130
🏁 Script executed:
#!/bin/bash
# Look at init functions in commands.go to see if workspace is added
rg -n 'func init\|AddCommand' src/cmd/cli/command/commands.go | tail -30Repository: DefangLabs/defang
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Find where all subcommands are registered to understand the full picture
rg -n 'RootCmd.AddCommand' src/cmd/cli/command/commands.goRepository: DefangLabs/defang
Length of output: 892
🏁 Script executed:
#!/bin/bash
# Let me trace the exact execution order and flag precedence
# When a subcommand is invoked, which flag takes precedence?
# Create a test to understand Cobra's behavior with persistent + local flags
python3 << 'EOF'
# Check Cobra's flag precedence behavior
# When both PersistentFlag (root) and Flag (subcommand) have same name,
# the local Flag shadows the PersistentFlag for that subcommand
# In workspace.go:
# - Line 65-66: registers local --json flags
# - Line 15: reads with cmd.Flags().GetBool("json") - gets LOCAL flag
# In commands.go:
# - Line 168: registers PersistentFlag --json on root
# - Line 366: reads with cmd.Flags().GetBool("json") - reads ROOT flag (in PersistentPreRunE)
# So:
# PersistentPreRunE (line 366): reads root's --json → calls SetJSONMode
# ListWorkspaces (line 15): reads local --json → uses for headers
# The question: do both get the same value?
# Answer: No! The local flag shadows the persistent flag
print("KEY INSIGHT:")
print("1. Root PersistentPreRunE reads root's --json flag (line 366)")
print("2. It calls SetJSONMode to set DEFANG_JSON env var")
print("3. ListWorkspaces reads LOCAL --json flag (line 15)")
print("4. Local flag can SHADOW root flag for this subcommand")
print()
print("SCENARIO: User runs 'defang workspace --json'")
print("- Root PersistentPreRunE: reads root flag (inherited by workspace)")
print("- But workspace has its own local --json flag (line 65)")
print("- The local flag definition shadows the inherited flag")
print("- So SetJSONMode gets the LOCAL flag value, not root flag")
print()
print("This means they ARE synced through Cobra's flag inheritance")
print("BUT: the local flags are REDUNDANT if the root PersistentFlag exists")
EOFRepository: DefangLabs/defang
Length of output: 706
🏁 Script executed:
#!/bin/bash
# Let me verify by checking if whoamiCmd has a similar pattern and if it's considered redundant
# Check whoami.go to see if it uses the same pattern
rg -n 'jsonMode\|--json' src/cmd/cli/command/whoami.goRepository: DefangLabs/defang
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Find and view whoami.go to see if it has similar pattern
find src/cmd/cli/command -name "whoami.go" -type f | xargs cat -nRepository: DefangLabs/defang
Length of output: 2484
🏁 Script executed:
#!/bin/bash
# Check the exact flow: when workspace --json is called, which flag is read in PersistentPreRunE?
# Let's verify by looking at the command hierarchy
rg -n 'PersistentPreRunE' src/cmd/cli/command/commands.go -A25 | grep -A25 "366"Repository: DefangLabs/defang
Length of output: 505
🏁 Script executed:
#!/bin/bash
# One final check: does the local flag in workspace.go actually get read by PersistentPreRunE?
# Or is there a disconnect?
python3 << 'EOF'
# Analysis of the code flow:
#
# commands.go line 355: PersistentPreRunE on RootCmd
# commands.go line 366: var json, _ = cmd.Flags().GetBool("json")
#
# When workspace subcommand is invoked:
# - PersistentPreRunE is called on RootCmd, which runs for ALL subcommands
# - At line 366, cmd is the subcommand (workspace) because PersistentPreRunE receives it
# - cmd.Flags().GetBool("json") looks for the flag in the subcommand's local flags FIRST
# then falls back to inherited persistent flags
#
# workspace.go lines 65-66: Define local --json flags
# workspace.go line 15: Same flag lookup in ListWorkspaces
#
# So the LOCAL flags DO get picked up by the PersistentPreRunE!
# The local flags are not redundant - they override/shadow the persistent flag for that subcommand
#
# But the question is: Is this intentional or a bug?
# - The comment in the original review suggests the local flags might be redundant
# - But actually, the local flags with pkg.GetenvBool("DEFANG_JSON") initial value
# ensure the flag state reflects the env var even before PersistentPreRunE runs
#
# CONCLUSION:
# The reviewer was concerned about inconsistency, but:
# 1. Local flags DO shadow persistent flags in Cobra
# 2. Local flags initialized with DEFANG_JSON env var value
# 3. Both ultimately control DEFANG_JSON env var through SetJSONMode
# 4. There's NO actual inconsistency - they work together properly
# 5. The local flags are NOT redundant - they serve to override at subcommand level
print("ANALYSIS RESULT:")
print("The original review comment about 'inconsistency' is INCORRECT")
print()
print("Facts:")
print("1. Root has PersistentFlag --json (line 168 in commands.go)")
print("2. Subcommands like workspace have local Flag --json (line 65-66)")
print("3. In Cobra, local flags shadow persistent flags")
print("4. Both are picked up by PersistentPreRunE at line 366")
print("5. SetJSONMode is called with the correct flag value")
print()
print("Result: NO inconsistency, NO redundancy")
print("The local flags are intentional to allow subcommand-level override")
EOFRepository: DefangLabs/defang
Length of output: 539
🏁 Script executed:
#!/bin/bash
# Final verification: Check if whoami also has local flag or just uses persistent
rg -n 'PersistentFlags.*json|Flags.*json' src/cmd/cli/command/whoami.goRepository: DefangLabs/defang
Length of output: 108
🏁 Script executed:
#!/bin/bash
# Check commands.go to see how whoami's flag is registered
rg -n -B2 -A2 'whoamiCmd.*PersistentFlags' src/cmd/cli/command/commands.goRepository: DefangLabs/defang
Length of output: 236
The local --json flags in workspace.go are picked up correctly and not redundant.
The local flags on workspaceCmd and workspaceListCmd (lines 65-66) do shadow the root's PersistentFlag. When PersistentPreRunE runs at line 366 of commands.go, cmd.Flags().GetBool("json") retrieves the local flag value from the workspace subcommand, which then gets passed to SetJSONMode. This properly sets the DEFANG_JSON environment variable that term.Table() checks at runtime, so there is no desync between what jsonMode reflects (line 39) and what term.Table outputs.
However, there is an inconsistency in how subcommands define their --json flags: workspace uses local Flags() while whoami uses PersistentFlags() on its command object (commands.go line 216). Since all flags ultimately work through the same SetJSONMode mechanism, consider standardizing this pattern across commands for clarity.
🤖 Prompt for AI Agents
In `@src/cmd/cli/command/workspace.go` around lines 39 - 41, The comment notes
inconsistent flag scoping: workspaceCmd and workspaceListCmd use local Flags()
for --json while the whoami command uses PersistentFlags(), which can confuse
readers; standardize the pattern by changing the subcommands to define the
--json flag the same way (either all use PersistentFlags() or all use Flags())
so SetJSONMode (invoked in PersistentPreRunE) and term.Table behavior remain
consistent; update the flag registration for workspaceCmd and workspaceListCmd
(or alternatively adjust whoami) to match the chosen approach and run the flag
parsing path through SetJSONMode to verify DEFANG_JSON is set as expected.
| } | ||
|
|
||
| EnableUTCMode() | ||
| SetUTCMode(true) |
There was a problem hiding this comment.
Missing cleanup: SetUTCMode(false) should be called to avoid test state leakage.
The PR objective mentions "avoid leaking utc and json mode between tests", but SetUTCMode(true) is called without registering a cleanup to restore the original state. This could affect subsequent tests.
🧹 Proposed fix to add cleanup
SetUTCMode(true)
+ t.Cleanup(func() { SetUTCMode(false) })
// Create the UTC time object📝 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.
| SetUTCMode(true) | |
| SetUTCMode(true) | |
| t.Cleanup(func() { SetUTCMode(false) }) | |
| // Create the UTC time object |
🤖 Prompt for AI Agents
In `@src/pkg/cli/tail_test.go` at line 251, SetUTCMode(true) is set in the test
without restoring it, causing global test state leakage; modify the test around
the SetUTCMode(true) call to restore the original state by registering a cleanup
that calls SetUTCMode(false) (or using defer if appropriate) so UTC mode is
reset after the test finishes; locate the call to SetUTCMode in tail_test.go and
add a t.Cleanup(func(){ SetUTCMode(false) }) (or equivalent) immediately after
it.
| filtered[i] = make(map[string]any) | ||
| for _, attr := range attributes { | ||
| field := item.FieldByName(attr) | ||
| if field.IsValid() { | ||
| filtered[i][attr] = field.Interface() | ||
| } | ||
| } |
There was a problem hiding this comment.
JSON output uses struct field names instead of json tags.
The current implementation uses the struct field name directly as the JSON key (e.g., "Provider", "SubscriberTier"), but the underlying structs like ShowAccountData have json tags defining different casing (e.g., json:"provider", json:"subscriberTier"). This creates inconsistent JSON output compared to what users might expect from the struct definitions.
Consider using the json tag if present, or document that JSON output uses PascalCase field names.
🔧 Proposed fix to respect json tags
filtered[i] = make(map[string]any)
for _, attr := range attributes {
field := item.FieldByName(attr)
if field.IsValid() {
- filtered[i][attr] = field.Interface()
+ // Use json tag if available, otherwise use field name
+ jsonKey := attr
+ if structField, ok := item.Type().FieldByName(attr); ok {
+ if tag := structField.Tag.Get("json"); tag != "" && tag != "-" {
+ jsonKey = strings.Split(tag, ",")[0]
+ }
+ }
+ filtered[i][jsonKey] = field.Interface()
}
}Note: This would require adding "strings" to the imports.
🤖 Prompt for AI Agents
In `@src/pkg/term/table.go` around lines 32 - 38, The JSON keys are using struct
field names (attr) instead of honoring json tags; update the loop that builds
filtered (reference variables: filtered, attributes, item, field) to look up the
StructField tag via reflect.Type.FieldByName(attr), read sf.Tag.Get("json"),
split on ',' and use the tag name if non-empty and not "-", otherwise fall back
to attr; assign filtered[i][jsonName] = field.Interface(); also add the
"strings" import for splitting the tag.
Description
Set utc and json mode in the root command instead of individual commands, and let
Tablewrite jsonLinked Issues
Checklist
Summary by CodeRabbit
New Features
--jsonand--utcflags at the root command level for consistent output formatting across all commands.Refactor
✏️ Tip: You can customize this high-level summary in your review settings.