fix: allow only valid types of the essential field

[lczyk]

• Have you signed the CLA?

---

this PR ensures that only valid types are allowed for the essential field: sequence in v1/v2 and map in v3

this is related to this discussion. tldr, we were upgrading chisel-releases branch ubuntu-26.04 to chisel.yaml v3 and therefore moving from v3-essential to essential. in the process we've accidentally made the following change:

package: libc6

essential:
-  - libc6_copyright
+  libc6_copyright:

slices:
  config:
    essential:
      # libc6 depends on the library paths set by base-files
-      - base-files_lib
+      base-files_lib:list

( .slices.config.essential is a string base-files_lib:list)

which chisel silently ignored and allowed the slices to be installed, causing lots of test failures. this was caught before merge, but should have been caught by chisel.

[github-actions]

Command	Mean [s]	Min [s]	Max [s]	Relative
BASE	13.514 ± 0.155	13.336	13.765	1.02 ± 0.01
HEAD	13.287 ± 0.068	13.157	13.398	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.

[lczyk]

reverted to e68b152 and added a link to this discussion in the test

[letFunny]

Reviewed, I added some questions, either way it looks good to me. Thank you for the work here!

[letFunny]

Thank you both for this! In my opinion it looks very good, I left two minor comments. Approving pending resolving them.

[letFunny]

Let's please rephrase the comment a little. I find "while the latter is valid from "v3" onwards." a little bit confusing because it is true that isList should be false for v3 but that condition is necessary but not sufficient. What we want to say is that format v3 expects isMap to be true and format <= v3 expects isList to be true. From the names we can see that they are exclusive or an alternative version is to have a simple enum of (UNKNOWN, MAP, LIST).

Anyway, for the comment I have the following suggestion just as a starting point but I don't mind the exact wording:

	// isList is set to true when the marshaler found a list and false otherwise.
	// A list is valid only in format "v1" and "v2".
	isList bool
	// isMap is set to true when the marshaler found a map and false otherwise.
	// A map is valid only from format "v3" onwards.

I think the wording makes it a bit more clear that they are exclusive and how they are related. Let me know what you think.

[upils]

Nice idea, thanks. I built on this suggestion, see 13668dc.

[lczyk]

i like the rework 👍

[letFunny]

Out of curiosity, any reason for moving from len() > 0 to != nil? They are almost equivalent but the problem in Go is that some collections have to be initialized while others don't (map has to be initialized, slice doesn't, you can just append). I have a slight nitpicky preference for using length as it is a bit more robust.

[upils]

The idea was to identify when UnmarshalYAML had been called but an empty value was set. Thanks for pointing that out, it made me think again about it and identify this approach was wrong. See the reworked one 13668dc.

[letFunny]

Thank you for all the changes. I think it is now much easier to read, the error messages are specific to the use-case and it is also more robust and preserves the same behavior we had in previous versions.