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.

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

cmd/cue: interaction between --merge(=false) and --path, --list and --files needs to be defined #3385

Open
myitcv opened this issue Aug 20, 2024 · 1 comment

Comments

@myitcv
Copy link
Member

myitcv commented Aug 20, 2024

What version of CUE are you using (cue version)?

$ cue version
cue version v0.0.0-20240819151828-81d6f8bfcd61

go version go1.23.0
      -buildmode exe
       -compiler gc
  DefaultGODEBUG asynctimerchan=1,gotypesalias=0,httpservecontentkeepheaders=1,tls3des=1,tlskyber=0,x509keypairleaf=0,x509negativeserial=1
     CGO_ENABLED 1
          GOARCH arm64
            GOOS linux
         GOARM64 v8.0
             vcs git
    vcs.revision 81d6f8bfcd61eeff2ec60ac71c8cc9867e6853e9
        vcs.time 2024-08-19T15:18:28Z
    vcs.modified false
cue.lang.version v0.11.0

Does this issue reproduce with the latest release?

Yes

What did you do?

cue help flags says the following about how non-CUE files are handled:

Non-CUE files are merged at their roots by default.
The can be combined differently or treated as different files
by using a combination of the following flags.

Individual files

To treat non-cue files as individual files, use --no-merge flag.
This is the default for vet. This flag only applies to data files
when used in combination with the --schema/-d flag.

Noting firstly that --no-merge does not exist as a flag: it is spelt --merge=false.

Also there doesn't appear to be a way to override the fact that vet does not merge non-CUE files:

b.mergeData = !b.cfg.noMerge && flagMerge.Bool(b.cmd)

The documentation above seems to imply that --merge applies at the non-CUE file-level. However this appears to be contradicted by the following:

exec cue export --merge=false a.jsonl
cmp stdout stdout.golden

-- a.jsonl --
{"test": "world"}
{"test": "hello"}
-- stdout.golden --
{
    "test": "world",
    "test": "hello"
}

This fails with:

> exec cue export --merge=false a.jsonl
[stdout]
{
    "test": "world"
}
{
    "test": "hello"
}
> cmp stdout stdout.golden
diff stdout stdout.golden
--- stdout
+++ stdout.golden
@@ -1,6 +1,4 @@
 {
-    "test": "world"
-}
-{
+    "test": "world",
     "test": "hello"
 }

FAIL: /tmp/testscript42942184/repro.txtar/script.txtar:2: stdout and stdout.golden differ

i.e. --merge=false appeared to result in the multiple values from a single non-CUE file not being "merged". This overloading however presents a problem when it comes to multiple multi-document non-CUE files - what should the result be then?

exec cue export --merge=false --path test a.jsonl b.jsonl
cmp stdout stdout.actual
cmp stdout stdout.golden

-- a.jsonl --
{"test": "world"}
{"test": "hello"}
-- b.jsonl --
{"test": "again"}
{"test": "hello"}
-- stdout.actual --
{
    "world": {
        "test": "world"
    },
    "again": {
        "test": "again"
    },
    "hello": {
        "test": "hello"
    }
}
-- stdout.golden --
{
    "world": {
        "test": "world"
    },
    "hello": {
        "test": "hello"
    }
}
{
    "again": {
        "test": "again"
    },
    "hello": {
        "test": "hello"
    }
}

This fails with:

> exec cue export --merge=false --path test a.jsonl b.jsonl
[stdout]
{
    "world": {
        "test": "world"
    },
    "again": {
        "test": "again"
    },
    "hello": {
        "test": "hello"
    }
}
> cmp stdout stdout.actual
> cmp stdout stdout.golden
diff stdout stdout.golden
--- stdout
+++ stdout.golden
@@ -2,6 +2,11 @@
     "world": {
         "test": "world"
     },
+    "hello": {
+        "test": "hello"
+    }
+}
+{
     "again": {
         "test": "again"
     },

FAIL: /tmp/testscript3899976859/repro2.txtar/script.txtar:3: stdout and stdout.golden differ

So now --merge=false does not produce multiple output values, it produces one with the merged result of the merged result from each multi-document file.

What did you expect to see?

Given the above explanation, unclear. We need to clarify the interaction of these flags and fix the implementation and documentation to match.

What did you see instead?

A confused user looking at me in the mirror.

There are various points that I think need clarifying here ahead of addressing #3369 and completing https://review.gerrithub.io/c/cue-lang/cue/+/1199496. Here is an incomplete list:

  • The current help text appears to imply that --merge has some effect on cue import for non-CUE files. Given that command is file-oriented in the first place, I'm not clear it means anything to specify (by default or otherwise) --merge. The existing --files does make sense for multi-document non-CUE files, regardless of how many are specified as arguments to cue import.
  • Deciding if --merge(=false) should only apply at the non-CUE file level, or whether it can also apply to the multiple values from multi-document files. Per above however, overloading in this way appears to conflate two apparently orthogonal concerns: 1) whether to merge the (multiple) values from multiple non-CUE files, 2) whether to merge the multiple values from a single multi-document non-CUE file.
  • Depending on the answer to the previous point, we need to ensure we have test coverage for the matrix of scenarios in which --merge can be used (answering the previous point might give birth to another flag). Specifically, we need to ensure that we have coverage for the case where we specify multiple multi-document files to, say, cue export. Because in that situation there are two "points" at which we might want to control a merge. The interaction (or lack thereof) with --path and --list (we ignore --files because that only applies to cue import) needs to be tested.
  • Documentation for all the above needs to be updated, potentially by updating https://review.gerrithub.io/c/cue-lang/cue/+/1199496.
  • Clarifying how --merge (and any related flags) should work, with whatever default, for vet and other commands.

It is highly likely that the above analysis is incomplete. I've started this issue mainly as a result of my response to https://review.gerrithub.io/c/cue-lang/cue/+/1199496 getting out of control!

@myitcv
Copy link
Member Author

myitcv commented Aug 20, 2024

In my attempt to make progress on the above analysis I was working on a matrix of cases:

  1. A single multi-document file
  2. Multiple multi-document files
  3. A single single-document file
  4. Multiple single-document files

In case it helps anyone on getting up to speed here, here are my WIP files:

case_1.txt
case_2.txt

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

No branches or pull requests

1 participant