Skip to content

fix(volume): resolve Network and Instance lookup fallbacks in volume ls (fixes #597) - #598

Merged
fulviodenza merged 3 commits into
civo:masterfrom
mahendrarathore1742:fix/issue-597-volume-list-lookups
Nov 2, 2026
Merged

fulviodenza merged 3 commits into
civo:masterfrom
mahendrarathore1742:fix/issue-597-volume-list-lookups

Conversation

@mahendrarathore1742

@mahendrarathore1742 mahendrarathore1742 commented Aug 6, 2026 •

Copy link
Copy Markdown
Contributor
### Summary
Fixes `civo volume ls` lookups for networks and instances so that unmatched items

safely fall back to raw IDs instead of showing unrelated names or breaking table column
alignment.

### Changes
- Replaced loop variable re-assignments with local fallback variables for network and

instance lookups.
- Ensured instance_id column is always appended once per row for consistent table
alignment.
- Added unit tests in cmd/volume/volume_list_test.go to verify matched and unmatched
fallback behavior.

Fixes #597

@fulviodenza fulviodenza left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for this, and sorry for the slow review. The fix is real: I ran the new TestVolumeListLookups against the current volume_list.go on master and it fails exactly as described in #597 (unmatched rows show the last network label / last hostname, and an empty instance when the cluster resolves but the node does not), and it passes with your change. Still exactly 4 list calls total, lookups are in memory, and a miss now falls back to the raw ID instead of a plausible wrong value. It also merges cleanly onto current master and the tests pass there.

Requested changes (small):

  1. Restore global state in the test (cmd/volume/volume_list_test.go): the test mutates config.Current, common.OutputFormat and common.OutputFields and redirects os.Stdout without restoring them. It is harmless today because it is the only test in the package, but it will leak into any test added later. Please save the old values and restore them with t.Cleanup, and handle the os.Pipe() error instead of discarding it.
  2. Rebase on master: the branch is a few months behind (civogo v0.7.2 vs v0.7.6 on master). No conflicts, but it would be good to have CI run on the current base.

Optional improvement: for volumes attached to cluster nodes where the volume's own cluster does not resolve, instance_id now shows the raw instance UUID. Since all clusters are already loaded, you could also search clusters.Items[*].Instances for the instance to show the node hostname, which is closer to what #597 asked for. Fine to leave for a follow-up.

The only CI run on this branch failed on a workflow file problem that predates the CI changes on master, so the rebase will also get it a proper run.

Address review feedback from @fulviodenza:
- Save and restore config.Current, common.OutputFormat, common.OutputFields,
  and os.Stdout using t.Cleanup to prevent state leaks into future tests
- Handle os.Pipe() error with t.Fatalf instead of discarding it
@mahendrarathore1742

Copy link
Copy Markdown
Contributor Author

Thanks for this, and sorry for the slow review. The fix is real: I ran the new TestVolumeListLookups against the current volume_list.go on master and it fails exactly as described in #597 (unmatched rows show the last network label / last hostname, and an empty instance when the cluster resolves but the node does not), and it passes with your change. Still exactly 4 list calls total, lookups are in memory, and a miss now falls back to the raw ID instead of a plausible wrong value. It also merges cleanly onto current master and the tests pass there.

Requested changes (small):

  1. Restore global state in the test (cmd/volume/volume_list_test.go): the test mutates config.Current, common.OutputFormat and common.OutputFields and redirects os.Stdout without restoring them. It is harmless today because it is the only test in the package, but it will leak into any test added later. Please save the old values and restore them with t.Cleanup, and handle the os.Pipe() error instead of discarding it.
  2. Rebase on master: the branch is a few months behind (civogo v0.7.2 vs v0.7.6 on master). No conflicts, but it would be good to have CI run on the current base.

Optional improvement: for volumes attached to cluster nodes where the volume's own cluster does not resolve, instance_id now shows the raw instance UUID. Since all clusters are already loaded, you could also search clusters.Items[*].Instances for the instance to show the node hostname, which is closer to what #597 asked for. Fine to leave for a follow-up.

The only CI run on this branch failed on a workflow file problem that predates the CI changes on master, so the rebase will also get it a proper run.

Thanks for the review @fulviodenza! Pushed the fix — global state is now saved and restored via t.Cleanup, and os.Pipe() error is handled. Will rebase on master for a clean CI run.

@fulviodenza

Copy link
Copy Markdown
Contributor

LGTM 🚀

@fulviodenza
fulviodenza merged commit fc1e0e6 into civo:master Nov 2, 2026
3 checks passed
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.

volume ls shows the wrong instance (and network): unmatched lookup falls through to the last item in the list

2 participants