Skip to content

fix: honour -o json/custom across missing and broken commands#586

Open
alubbock wants to merge 8 commits into
civo:masterfrom
alubbock:fix/k8s-nodepool-instance-delete-json-output
Open

fix: honour -o json/custom across missing and broken commands#586
alubbock wants to merge 8 commits into
civo:masterfrom
alubbock:fix/k8s-nodepool-instance-delete-json-output

Conversation

@alubbock
Copy link
Copy Markdown
Contributor

Several commands ignored the -o json and -o custom flags, always
printing plain text regardless. One command (kubernetes cluster remove)
had a latent bug where -o json also triggered the custom formatter.

Missing output format handling (always printed plain text):

  • kubernetes node-pool instance-delete
  • kubernetes cluster remove (also had WriteCustomOutput inside the
    "json" case, so -o json produced both outputs)
  • apikey remove / apikey set
  • instance public-ip / instance recovery
  • kubernetes update-kubeconfig
  • network show

Also fixed in instance recovery: a fmt.Println("Instance ID is: …") that fired before the API call, unconditionally, on every
invocation.

@giornetta giornetta self-requested a review May 12, 2026 08:38
ow.AppendDataWithLabel("status", network.Status, "Status")
ow.AppendDataWithLabel("ipv4_enabled", utility.BoolToYesNo(network.IPv4Enabled), "IPv4 Enabled")
ow.AppendDataWithLabel("ipv6_enabled", utility.BoolToYesNo(network.IPv6Enabled), "IPv6 Enabled")
ow.AppendDataWithLabel("vlan_id", fmt.Sprintf("%d", network.VlanID), "VLAN ID")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The fields vlan_id, physical_interface, allocation_pool_v4_start and allocation_pool_v4_end should only be displayed if the network is VLAN-enabled.

This way, vlan_id is shown in the JSON output as "vlan_id":"0", whereas the other fields are silently dropped because they're empty. I would add a guard here to prevent vlan_id from appearing in the JSON output as it's happening in the human formatting.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Also a minor nitpick, we're using strconv.Itoa(network.VlanID) in network_list.go, we should probably stick to that for consistency.

fmt.Println("\nVLAN Details:")
fmt.Printf("VLAN ID: %d\n", network.VlanID)
fmt.Printf("Hardware Address: %s\n", network.PhysicalInterface)
fmt.Printf("Gateway IPv4: %s\n", network.GatewayIPv4)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is actually a pre-existing "bug" we could solve here: we can show the gateway IP even for non-VLAN networks, so this can be moved outside the guard.

utility.Error("%s", err)
os.Exit(1)
}
fmt.Println("Instance ID is: ", instance.ID)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

By removing this line, the human-form output no longer shows the UUID of the instance, but only the hostname. Consider adding it (between parentheses?) in the human-form output, next to the hostname

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants