fix: allow only map and sequence essential field

[lczyk]

error if essential field is not a sequence or a map

• see feat(26.04): introduce v3 format chisel-releases#903 (comment)

• Have you signed the CLA?

[github-actions]

Command	Mean [s]	Min [s]	Max [s]	Relative
BASE	14.999 ± 0.598	14.125	15.771	1.01 ± 0.04
HEAD	14.825 ± 0.254	14.252	15.160	1.00

[letFunny]

Thank you for this @lczyk, indeed we paid so much attention to get the error messages right for list and map that we didn't catch the case when the node is neither. I added a suggestion to change the error message to follow the convention and then this is good to go.

[lczyk]

spoke with @upils and while this fixes the bug, the validation behind the scenes is a bit of a spaghetti and so the error message is not quite as pleasant as we might wish it were. ideally it'd be

v1/v2: cannot parse package "mypkg": essential field must be a list
   v3: cannot parse package "mypkg": essential field must be a map
  not: cannot parse package "mypkg" slice definitions: essential field must be a list or a map

i've fixed the tests to pass with the sub-optimal error message just so the PR is good, but @upils might end up rolling their own PR to try to make the error messages better 👍

[lczyk]

i've had to adjust oldEssentialToV3 since it was patching away my tests

[upils]

Thanks!

[letFunny]

Last time @upils and rest of the team agreed that we want to stop having a complex test processing step and instead we want to have individual tests. What about adding a test for format v1 / v3 explicitly instead.

[upils]

@letFunny This is a format-independent test, so there is little point in writing 2 tests. Duplicated test will have to be tracked down when we is removed oldEssentialToV3, so the lesser the better I think. In a previous iteration from @lczyk oldEssentialToV3 was in fact getting in the way so fixing it for it to accept this "common" test makes sense.

[letFunny]

@upils

This is a format-independent test

Yes, indeed, which is why it doesn't matter if it is tested with v3 or v1, wouldn't you agree? We agreed on quite the opposite actually, we want to test the latest versions when possible and have specific tests for past versions (per our discussion with Gustavo and I very much agree). So, why not include a single test with v3, because it is format independent, and avoid adding even more complexity to the translation code we want to delete?

This works:

diff --git a/internal/setup/setup_test.go b/internal/setup/setup_test.go
index 95a3d5b..608b0f0 100644
--- a/internal/setup/setup_test.go
+++ b/internal/setup/setup_test.go
@@ -3728,6 +3728,18 @@ var setupTests = []setupTest{{
 		`,
 	},
 	relerror: `cannot parse package "mypkg": essential expects a list`,
+}, {
+	summary: "Essential must be list or map",
+	input: map[string]string{
+		"chisel.yaml": strings.ReplaceAll(testutil.DefaultChiselYaml, "format: v1", "format: v3"),
+		"slices/mydir/mypkg.yaml": `
+			package: mypkg
+			essential: not-a-list-or-map
+			slices:
+				myslice:
+		`,
+	},
+	relerror: `cannot parse package "mypkg" slice definitions: cannot decode essential field`,
 }, {
 	summary: "Format v1/v2 expect a list in 'essential' (slice)",
 	input: map[string]string{
diff --git a/internal/setup/yaml.go b/internal/setup/yaml.go
index b048bfc..04f4950 100644
--- a/internal/setup/yaml.go
+++ b/internal/setup/yaml.go
@@ -95,6 +95,8 @@ func (es *yamlEssentialListMap) UnmarshalYAML(value *yaml.Node) error {
 		if err != nil {
 			return err
 		}
+	default:
+		return fmt.Errorf("cannot decode essential field")
 	}
 	es.Values = m
 	return nil

And it is much more simple, wouldn't you agree? This is commit e68b152, credit where credit is due.

[upils]

Yes, indeed, which is why it doesn't matter if it is tested with v3 or v1, wouldn't you agree?

I agree it should not matter and we know we can test only for v3 because we are aware of the implementation.
By "format-independent" I meant "it must work the same way for all formats" and not "we know this is implemented the same way for all format". This is why, with the current strategy, it made sense to me to use a test that is "mutated" by oldEssentialToV3.

I also agree with the approach discussed with Gustavo. I assume that when we rework this:

• the "common" tests will, by default, become tests for the last format
• test currently "forced" to be be on V3 will become V3-specific tests, testing features only supported in V3 (and same for other formats).

So with these assumptions in mind, I thought declaring this new test as a format-agnostic test made the intent clearer.

In the end I am fine with either solutions because anyway all tests will have to be carefully reviewed when simplifying that, so one more test maybe not in the right "group" might not be a big deal.