fix(parse)!: Remove csv-style parsing for lists and maps by nurof3n · Pull Request #395 · unikraft-cloud/cli

nurof3n · 2026-07-03T12:54:02Z

Closes TOOL-1125

Copilot

Pull request overview

This PR updates instance-related commands to correctly accept environment variables in KEY=VALUE form even when values contain commas, by preventing Kong’s default comma-splitting and handling parsing explicitly before shortcut-flag processing.

Changes:

Set sep:"none" for -e/--env on instance create/edit to avoid Kong splitting values on commas.
Pre-process Env into --set runtime.env=... entries before applying generic shortcut flags (create/edit/run).
Add helpers to split and encode env assignments as JSON map fragments for consistent patch parsing.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
internal/cmd/run.go	Pre-processes `--env` into `--set` entries before generic shortcut handling to support `KEY=VALUE` parsing.
internal/cmd/instances.go	Disables Kong comma-splitting for `--env`, adds env parsing helpers, and applies the same pre-processing for create/edit.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

craciunoiuc

Some comments my side 🥺

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

craciunoiuc · 2026-07-07T13:08:52Z

@nurof3n tests failing 😳

Also please close copilot comments which no longer apply

Looks much leaner now 🫦

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

Copilot

Pull request overview

Copilot reviewed 21 out of 21 changed files in this pull request and generated no new comments.

Suppressed comments (3)

internal/cmd/instances.go:265

This drops the previous strings.TrimSpace normalization from InstanceService.UnmarshalText. multimetro.ParseKey does not trim its input, so a value such as --service " my-service " now produces a link whose name contains spaces and fails lookup. Preserve the existing normalization before parsing the link.

	link, err := ParseLink[ServiceGroup](data)

internal/resource/patch/visual.go:229

Please add a regression test for the new deepest-first behavior. The current visual-edit tests cover nested child fields, but not a settable ancestor sharing the same YAML subtree with a settable descendant—the exact case this ordering fixes. A service-like fixture should verify that the ancestor's own link/name and a descendant collection both survive deserialization, and that unknown nested fields are still rejected.

	slices.SortStableFunc(entries, func(a, b fieldEntry) int {
		return cmp.Compare(len(b.key), len(a.key))
	})

internal/cmd/volumes.go:110

Integration coverage is incomplete for the newly renamed volume clone --tag shortcut; the existing volume integration only exercises --tag during create, while clone has its own manual patch-to-request path. Please create a source volume, clone it with repeated --tag flags, inspect that the clone has distinct tags (including the literal-comma behavior), and clean up both volumes.

	Tag  []string `group:"flag-clone" shortcut:"tags" sep:"none" help:"Volume tag." placeholder:"tag" example:"env-prod"`

craciunoiuc

All good here. Thanks!

Reviewed-by: Cezar Craciunoiu <cezar.craciunoiu@unikraft.com>
Approved-by: Cezar Craciunoiu <cezar.craciunoiu@unikraft.com>

craciunoiuc · 2026-08-20T06:53:37Z

that's a lot of edge cases, now I'm scared there are more 👀

jedevc · 2026-08-20T07:05:11Z

The last commit is a pre-existing issue, I can split to a separate PR tbf.

Copilot

Pull request overview

Copilot reviewed 24 out of 24 changed files in this pull request and generated 1 comment.

Suppressed comments (9)

internal/cmd/volumes.go:110

The new volume clone --tag interface has no integration coverage; the clone test still uses only --set name. Please clone with repeated --tag, inspect the clone to confirm separate tag values and success, cover rejection of the removed --tags spelling, and delete both clone and source.

	Tag  []string `group:"flag-clone" shortcut:"tags" sep:"none" help:"Volume tag." placeholder:"tag" example:"env-prod"`

internal/resource/value/parse.go:155

This comment contradicts the implementation and the new map key with no value test: field= has a non-empty key and adds field: "". The skipped form is an entry such as =value. Please describe that case so future callers do not rely on incorrect parsing semantics.

			// not from splitting one input. Only the key is trimmed, and an
			// empty key is skipped so a bare "field=" adds no entry.

internal/cmd/services.go:79

Integration coverage is incomplete for the renamed service edit shortcuts: current integration tests exercise repeated --domain/--service only during creation, while edit still uses --set. Please add an end-to-end edit using repeated singular flags, inspect the resulting domains/ports and exit status, verify the removed plural flags are rejected, and clean up the service group.

	Domain  []Domain  `group:"flag-edit" shortcut:"domains" sep:"none" help:"Service domain." placeholder:"fqdn" example:"example.com"`
	Service []Service `group:"flag-edit" shortcut:"services" sep:"none" help:"Service port." placeholder:"<src>:<dest>[/<handlers>]" example:"443:8080/http+tls"`

internal/cmd/volumes.go:94

Integration coverage is missing for the renamed volume edit shortcut. Please edit a live volume with repeated --tag, verify inspect/list observes separate tags and a successful exit, check that the removed --tags form is rejected, and clean up the volume.

This issue also appears on line 110 of the same file.

	Tag         []string            `group:"flag-edit" shortcut:"tags" sep:"none" help:"Volume tag." placeholder:"tag" example:"env-prod"`

internal/cmd/instances.go:152

Integration coverage is missing for the renamed instance edit --tag shortcut; existing tag scenarios use it only on create and use --set for edits. Please edit a live instance with repeated --tag, verify the resulting separate tags and exit status, assert that legacy --tags is rejected, and delete the instance.

	Tag []string `group:"flag-edit" shortcut:"tags" sep:"none" help:"Instance tag." placeholder:"tag" example:"env-prod"`

internal/cmd/instance_templates.go:64

The user-facing template edit flag is renamed to --tag, but the integration test still edits tags exclusively through repeated --set. Please exercise repeated --tag end to end, inspect/filter for both separate values, verify --tags is rejected, and retain the existing template/resource cleanup.

	Tag        []string `group:"flag-edit" shortcut:"tags" sep:"none" help:"Template tag." placeholder:"tag" example:"env-dev"`

internal/cmd/volume_templates.go:64

The renamed volume-template --tag shortcut lacks integration coverage because the current test uses --set tags=.... Please edit a template with repeated --tag, inspect/filter to prove the values remain separate, cover rejection of --tags, and preserve cleanup of the template and source volume.

	Tag        []string `group:"flag-edit" shortcut:"tags" sep:"none" help:"Template tag." placeholder:"tag" example:"env-dev"`

internal/cmd/instance_checkpoints.go:67

The checkpoint edit flag now exposes --tag, but integration coverage still goes through repeated --set. Please edit a checkpoint with repeated --tag, inspect/filter for both separate tags and a successful exit, verify the removed --tags spelling fails, and clean up the checkpoint and instance.

	Tag        []string `group:"flag-edit" shortcut:"tags" sep:"none" help:"Checkpoint tag." placeholder:"tag" example:"env-dev"`

internal/resource/patch/visual.go:229

The nested visual-editing behavior is covered only by a unit test, but it changes the end-to-end CLI path. Please add an integration case that creates an instance with --service, repeated nested --domain/--publish, and a noninteractive editor such as --cmd cat; then inspect the service link, domains, and ports, assert the exit status, and clean up the instance/service resources.

	slices.SortStableFunc(entries, func(a, b fieldEntry) int {
		return cmp.Compare(len(b.key), len(a.key))
	})

+	data, err := yaml.Marshal(v)
+	if err != nil {
+		return map[string]any{}


craciunoiuc · 2026-08-20T07:41:14Z

nah, no need

but do check if there are any cases left

I will also do a pass soon, might start a robot in parallel to look for any edge cases

craciunoiuc · 2026-08-20T08:19:09Z

There are some weird comments but today I feel like that sugar should not be hoisted on the chopping block

PR looks ok, I'm testing this cool extension and then reporting what it found

craciunoiuc · 2026-08-20T08:57:00Z

static check failed, in case you are wondering why, it's because golangci-lint got updated and the new version has more checks

in X it's better to pin down, but here maybe we can live with the bump?

that being said, should the fix be here or in a separate PR?

jedevc · 2026-08-20T09:11:19Z

Let's fix it separately, yes. I'll pin it down in x.

craciunoiuc

All good here. Thanks!

Reviewed-by: Cezar Craciunoiu <cezar.craciunoiu@unikraft.com>
Approved-by: Cezar Craciunoiu <cezar.craciunoiu@unikraft.com>

Signed-off-by: Alex-Andrei Cioc <andrei.cioc@unikraft.io>

And rename flags to make this clearer. So `--tags` becomes `--tag`, etc. This is a major breaking change, however, the end result should be significantly clearer. In the future, we should work out how to remove the CSV values for struct parsing as well, however, this is much trickier, especially as we are dependent on this in more places. Signed-off-by: Justin Chadwell <justin@unikraft.com>

Signed-off-by: Justin Chadwell <justin@unikraft.com>

craciunoiuc

All good here. Thanks!

Reviewed-by: Cezar Craciunoiu <cezar.craciunoiu@unikraft.com>
Approved-by: Cezar Craciunoiu <cezar.craciunoiu@unikraft.com>

nurof3n requested a review from jedevc July 3, 2026 12:54

nurof3n force-pushed the nurof3n/fix/instances-env-sep branch from 2e442b8 to 89f1bc0 Compare July 6, 2026 13:05

craciunoiuc requested a review from Copilot July 6, 2026 18:08

Copilot started reviewing on behalf of craciunoiuc July 6, 2026 18:08 View session

Copilot AI reviewed Jul 6, 2026

View reviewed changes

Comment thread internal/cmd/instances.go Outdated

craciunoiuc requested changes Jul 6, 2026

View reviewed changes

Comment thread internal/cmd/instances.go

Comment thread internal/cmd/instances.go Outdated

Comment thread internal/cmd/instances.go Outdated

jedevc reviewed Jul 7, 2026

View reviewed changes

Comment thread internal/cmd/instances.go Outdated

nurof3n force-pushed the nurof3n/fix/instances-env-sep branch from 89f1bc0 to 5311264 Compare July 7, 2026 12:48

nurof3n requested review from Copilot, craciunoiuc and jedevc July 7, 2026 12:48

Copilot started reviewing on behalf of nurof3n July 7, 2026 12:49 View session

Copilot AI reviewed Jul 7, 2026

View reviewed changes

Comment thread internal/cmd/instances.go Outdated

Comment thread internal/resource/value/parse_test.go Outdated

Comment thread internal/resource/value/parse.go Outdated

nurof3n force-pushed the nurof3n/fix/instances-env-sep branch from 5311264 to a8d856f Compare July 7, 2026 13:00

nurof3n requested a review from Copilot July 7, 2026 13:00

Copilot started reviewing on behalf of nurof3n July 7, 2026 13:00 View session

Copilot AI reviewed Jul 7, 2026

View reviewed changes

Comment thread internal/resource/value/parse.go

Comment thread internal/resource/value/parse_test.go Outdated

Comment thread internal/cmd/instances.go Outdated

nurof3n changed the title ~~fix(instances): Handle env variables with key=value, syntax~~ fix(instances): Preserve env variables verbatim Jul 7, 2026

nurof3n changed the title ~~fix(instances): Preserve env variables verbatim~~ fix(instances): Preserve env variables values verbatim Jul 7, 2026

nurof3n force-pushed the nurof3n/fix/instances-env-sep branch from a8d856f to 937a9a1 Compare July 7, 2026 13:26

nurof3n requested a review from Copilot July 7, 2026 13:27

Copilot started reviewing on behalf of nurof3n July 7, 2026 13:28 View session

Copilot AI reviewed Jul 7, 2026

View reviewed changes

Comment thread internal/resource/value/parse.go Outdated

craciunoiuc reviewed Jul 7, 2026

View reviewed changes

Comment thread internal/resource/value/parse.go Outdated

craciunoiuc reviewed Jul 7, 2026

View reviewed changes

Comment thread internal/resource/value/parse_test.go Outdated

jedevc reviewed Jul 7, 2026

View reviewed changes

Comment thread internal/resource/value/parse.go Outdated

nurof3n requested review from Copilot, craciunoiuc and jedevc July 8, 2026 11:11

jedevc dismissed craciunoiuc’s stale review via e3747f8 August 19, 2026 15:52

jedevc requested a balanced review from Copilot August 19, 2026 15:56

Copilot started reviewing on behalf of jedevc August 19, 2026 15:56 View session

jedevc force-pushed the nurof3n/fix/instances-env-sep branch from e3747f8 to e6c6fd6 Compare August 19, 2026 15:56

Copilot AI reviewed Aug 19, 2026

View reviewed changes

jedevc force-pushed the nurof3n/fix/instances-env-sep branch from e6c6fd6 to 4ceefde Compare August 19, 2026 16:27

craciunoiuc previously approved these changes Aug 19, 2026

View reviewed changes

jedevc dismissed craciunoiuc’s stale review via 70f937d August 19, 2026 22:50

jedevc requested a balanced review from Copilot August 20, 2026 07:04

Copilot started reviewing on behalf of jedevc August 20, 2026 07:04 View session

Copilot AI reviewed Aug 20, 2026

View reviewed changes

Comment thread internal/resource/patch/visual.go Outdated

Comment on lines +345 to +347

data, err := yaml.Marshal(v)

if err != nil {

return map[string]any{}

jedevc force-pushed the nurof3n/fix/instances-env-sep branch from 70f937d to f607120 Compare August 20, 2026 07:42

craciunoiuc approved these changes Aug 20, 2026

View reviewed changes

nurof3n and others added 5 commits August 20, 2026 11:50

fix(instances): Preserve env variables values verbatim

2d18a55

Signed-off-by: Alex-Andrei Cioc <andrei.cioc@unikraft.io>

fix(parse): Take separator into account

1903ec1

Signed-off-by: Alex-Andrei Cioc <andrei.cioc@unikraft.io>

fix(parse): Make all tests pass

3eec6c7

Signed-off-by: Justin Chadwell <justin@unikraft.com>

fix(parse): Handle some more edge cases

c82c772

Signed-off-by: Justin Chadwell <justin@unikraft.com>

jedevc force-pushed the nurof3n/fix/instances-env-sep branch from f607120 to c82c772 Compare August 20, 2026 10:50

craciunoiuc approved these changes Aug 20, 2026

View reviewed changes

jedevc merged commit 630b331 into prod-staging Aug 20, 2026
13 checks passed

jedevc deleted the nurof3n/fix/instances-env-sep branch August 20, 2026 17:03

This was referenced Aug 20, 2026

fix: Catch up docs with the CLI flag renames unikraft-cloud/docs#261

Merged

fix(volume-templates): Create a template per volume #460

Merged

Uh oh!

Conversation

nurof3n commented Jul 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

craciunoiuc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

craciunoiuc commented Jul 7, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

craciunoiuc left a comment

Choose a reason for hiding this comment

Uh oh!

craciunoiuc commented Aug 20, 2026

Uh oh!

jedevc commented Aug 20, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

craciunoiuc commented Aug 20, 2026

Uh oh!

craciunoiuc commented Aug 20, 2026

Uh oh!

craciunoiuc commented Aug 20, 2026

Uh oh!

jedevc commented Aug 20, 2026

Uh oh!

craciunoiuc left a comment

Choose a reason for hiding this comment

Uh oh!

craciunoiuc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nurof3n commented Jul 3, 2026 •

edited

Loading