fix: allow only map and sequence essential field

[lczyk]

• Have you signed the CLA?

---

this PR adds an error condition if essential field is not a sequence or a map.

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]

Sorry for missing this in earlier reviews, I only had checked the overall approach and not the specifics. Paul asked me to review today so I played around with the code a little bit more. I think this looks good but I wonder if we could do better. What is instead of the generic error message list or map which is not applicable to either v3 or v1, we had two variables isList and isMap where both can be true or false. Then each version can return the appropriate error by saying expected map or expected list on the existing error path.

I am not saying to do this but to just think if it is an valid option or not. If it is much more complex then we don't need it, it is just a nice to have.

Also, IMO even with this change, the test that is currently in the PR is all that is needed to test this new error path.

[letFunny]

Nit: I don't think this is relevant for the tests or for the people reading them. Right now the problem is that it doesn't follow the existing tests style, but it does follow the new rule, so IMO it is expected and not saying we need to highlight.