feat: format v3 #249

letFunny · 2025-10-17T14:11:26Z

Format v3 uses a map instead of a list for essential and deprecates the field v3-essential which is only used during the transition from v2 to v3.

Have you signed the CLA?

Format v3 uses a map instead of a list for `essential` and deprecates the field `v3-essential` which is only used during the transition from v2 to v3.

internal/setup/yaml.go

github-actions · 2025-10-17T14:17:24Z

Command	Mean [s]	Min [s]	Max [s]	Relative
`BASE`	12.999 ± 0.411	12.263	13.581	1.04 ± 0.04
`HEAD`	12.555 ± 0.281	12.228	13.258	1.00

niemeyer

On a first pass, as detailed below it's surprising and unfortunate that this PR seems to make things significantly worse almost everywhere it touches. This was supposed to be a cleanup PR ideally.

internal/setup/yaml.go

upils · 2026-01-13T14:20:11Z

internal/setup/yaml.go

 	},
 }
+
+func parseSliceEssential(yamlPkg *yamlPackage, yamlSlice *yamlSlice, slice *Slice) error {


This refactoring makes it hard to track what changed compared to the previous implementation. Could we do the extraction in a dedicated function in another PR?

I thought about that but it is a bit overkill because what happens if we end up changing the approach, then we merged a PR for a refactor that wasn't actually fully needed. As I told you offline, this function needs a bit of love to make it more readable but, if done properly, I think it is easy to see the logic just moved. I will make the changes and we can discuss.

I think with the latest changes it is easier to see that the diff is the same.

internal/setup/yaml.go

letFunny · 2026-01-15T11:11:17Z

As discussed with @niemeyer this week and the previous one, I have changed the PR. The current approach does not need two separate definitions of the same type and instead just changes how the conflicting field is parsed (in > v3 as a map, in v1, v2 as a list). This solution is not as strict as the previous one in regards to chisel info but as agreed it was a good tradeoff.

The complexity that still exists in the current approach is mainly because we need to produce good error messages and discern between all combinations of format + fields.

For the record, Paul and me spent even more time this week thinking about the problem and we also tried:

Embedding the struct. It is far more complex because Unmarshal gets the full yaml.Node and needs to write a custom parser for the whole struct, or we are back at duplicating type definitions.
Do the parsing of V3Essential and Essential in Unmarshal together is more complex because the format is needed to produce good errors. Adding information in Unmarshal is not simple when structs are created dynamically.
Have two different types that support v1/v2 and v3. Discarded in the previous PR based on Gustavo's comment.
Have two types with structs embedded. We also discarded it for the similarity with the previous one.
Combinations of the previous and cosmetic differences.

upils

Thanks @letFunny. I have a couple of questions, mainly about tests, but overall it LGTM.

internal/setup/yaml.go

internal/setup/setup_test.go

upils · 2026-01-15T12:39:25Z

internal/setup/setup_test.go

+// 'v3-essential' to the shape expected by the v3 format.
+// skip is set to true when an accurate translation of the test is not
+// possible, for example having duplicates in the list.
+func oldEssentialToV3(c *C, input []byte) (out string, skip bool) {


[nitpick]: This is a clever solution to avoid duplicating the test cases but given the complexity I find it a bit hard to know what the final input to the test is.
I also wonder if we really need to rerun this many tests when many of them are not testing the essential resolution logic. I guess we are better safe than sorry but I wonder how hard that will be to maintain, especially when adding new tests.

Yeah, related to my earlier comment above. I would like to do the opposite: keep tests compatible with the latest version, and backport it to the old ones. Eventually we can just drop the backports.

Or, alternatively, drop this whole multi-version logic and have just specific tests that cover the difference. Yeah, that's probably better actually.

niemeyer

This is looking good, thank you. A few comments:

niemeyer · 2026-01-19T17:02:03Z

internal/setup/setup.go

 	Packages    map[string]*Package
 	Archives    map[string]*Archive
 	Maintenance *Maintenance
+	// Format is used for parsing and error checking.


This is documenting the type, so the focus here is on what it actually is, not where it is used. It is going to be used wherever it is needed, and this information will be out of date soon.

The Format must also be moved as the first item as everything else follows from it.

niemeyer · 2026-01-19T17:05:58Z

internal/setup/setup_test.go

 	},
 	release: &setup.Release{
+		Format: "v1",
 		Archives: map[string]*setup.Archive{


Per above note, we always end up thinking as Format as the first thing.

niemeyer · 2026-01-19T17:07:58Z

internal/setup/setup_test.go

 		`,
 	},
-	relerror: `cannot add slice to itself as essential "mypkg_slice1" in slices/mydir/mypkg.yaml`,
+	relerror: `package "mypkg": cannot add slice to itself as essential "mypkg_slice1"`,


Can we please not fiddle with unrelated messages unless it's clearly a fix or improvement? Some of these messages have been extensively discussed, and this is one example where it is clearly making it significantly worse and out of convention, and this is all a distraction given the main topic of the PR.

My intention was to make them fit with the convention I saw elsewhere in the code as we have slightly different error messages when "decoding" from yaml vs when we are parsing or validating the contents. But you are right here, let's not discuss it as part of this PR.

niemeyer · 2026-01-19T17:16:11Z

internal/setup/setup_test.go

 	runParseReleaseTests(c, v2FormatTests)
+
+	// Run tests for "v3" format.
+	v3FormatTests := make([]setupTest, 0, len(setupTests))


In a follow up PR, can we please invert this whole logic so that the tests are actually reflecting v3, and then we fiddle backwards instead of forwards? Otherwise we keep adding more work to the tip, which is where we care the most about, and eventually we'll have an issue because we'll want to drop v1.

Discussed offline: agree on testing v3. About the other point of manually testing, it comes a point where the testing code is so complex that we are not sure it is correct anymore. We can rely on the knowledge about the difference between v1, v2 and v3.

niemeyer · 2026-01-19T17:17:29Z

internal/setup/setup_test.go

+// 'v3-essential' to the shape expected by the v3 format.
+// skip is set to true when an accurate translation of the test is not
+// possible, for example having duplicates in the list.
+func oldEssentialToV3(c *C, input []byte) (out string, skip bool) {


Yeah, related to my earlier comment above. I would like to do the opposite: keep tests compatible with the latest version, and backport it to the old ones. Eventually we can just drop the backports.

Or, alternatively, drop this whole multi-version logic and have just specific tests that cover the difference. Yeah, that's probably better actually.

niemeyer · 2026-01-19T17:36:00Z

internal/setup/yaml.go

+	m := map[string]*yamlEssential{}
+	es.isList = false
+	err := value.Decode(&m)
+	if err != nil {


We have the yaml.Node, so we should be able to tell whether it has the right type or not without assuming that we understand what the arbitrary error is. This should allow us to switch on the type and have more precise errors.

niemeyer · 2026-01-19T17:36:39Z

internal/setup/yaml.go

+		es.isList = true
+		for _, sliceName := range l {
+			if _, ok := m[sliceName]; ok {
+				return fmt.Errorf("cannot decode essential: repeats %s", sliceName)


This is not a decoding error. We had a nice message before. Let's please make sure the entire composition is still good and clear.

Discussed offline, this is not a decoding error. Find the original error message and adapt it.

niemeyer · 2026-01-19T17:43:48Z

internal/setup/yaml.go

 	V3Essential map[string]*yamlEssential `yaml:"v3-essential,omitempty"`
+	// For backwards-compatibility reasons with v1 and v2, essential needs
+	// custom logic to be parsed. See [yamlEssentialListMap].
+	Essential yamlEssentialListMap `yaml:"essential,omitempty"`


Let's please keep the original order, and give the decoding one the odd name. Essential should be what we want to think of the essential field when we're dealing with it on v3.

niemeyer · 2026-01-19T17:44:47Z

internal/setup/yaml.go

+		}
+	} else {
+		if yamlPkg.V3Essential != nil {
+			return nil, fmt.Errorf("cannot parse package %q: v3-essential is deprecated since format v3", pkgName)


s/deprecated/obsolete/

Deprecation is typically something that still works but shouldn't be used.

niemeyer · 2026-01-19T18:09:48Z

internal/setup/yaml.go

+			return fmt.Errorf("package %q has invalid essential slice reference: %q", yamlPkg.Name, refName)
+		}
+		if sliceKey.Package == slice.Package && sliceKey.Slice == slice.Name {
+			return fmt.Errorf("package %q: cannot add slice to itself as essential %q", yamlPkg.Name, refName)


Why is the refName here not a %s? Those are slices, right? So other than validating the slice name itself, which is done above, we can trust it to be right. That's unlike package name, which needs %q as it may be a word and get confused in the sentence.

niemeyer

Comments are above.

feat: format v3

d70d273

Format v3 uses a map instead of a list for `essential` and deprecates the field `v3-essential` which is only used during the transition from v2 to v3.

letFunny requested a review from niemeyer October 17, 2025 14:11

typo

e8b236b

letFunny commented Oct 17, 2025

View reviewed changes

internal/setup/yaml.go Show resolved Hide resolved

internal/setup/yaml.go Show resolved Hide resolved

niemeyer requested changes Oct 20, 2025

View reviewed changes

internal/setup/yaml.go Show resolved Hide resolved

internal/setup/yaml.go Show resolved Hide resolved

internal/setup/yaml.go Outdated Show resolved Hide resolved

internal/setup/yaml.go Outdated Show resolved Hide resolved

letFunny added 7 commits January 13, 2026 11:20

checkpoint

e6d26fb

working

9401f01

refactor

821300c

more robust checking

a4fb047

more tests

6e00778

comment

42910ea

move

8246bb6

upils reviewed Jan 14, 2026

View reviewed changes

letFunny commented Jan 14, 2026

View reviewed changes

internal/setup/yaml.go Outdated Show resolved Hide resolved

letFunny commented Jan 14, 2026

View reviewed changes

internal/setup/yaml.go Outdated Show resolved Hide resolved

letFunny added 6 commits January 15, 2026 09:54

rename

3dc463b

abstract common functions

eb60e15

remove unneeded arg

2df907c

rename yamlEssentialListMap

d2d6799

move for consistency with other types

5acd1d7

Merge branch 'main' into v3-format

5bf9473

letFunny requested review from niemeyer and upils January 15, 2026 11:04

upils approved these changes Jan 15, 2026

View reviewed changes

address review

d4b4097

letFunny added the Priority Look at me first label Jan 15, 2026

niemeyer reviewed Jan 19, 2026

View reviewed changes

niemeyer requested changes Jan 19, 2026

View reviewed changes

feat: format v3 #249

Are you sure you want to change the base?

feat: format v3 #249

Conversation

letFunny commented Oct 17, 2025

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Oct 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

niemeyer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

letFunny commented Jan 15, 2026

Uh oh!

upils left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

niemeyer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

letFunny Jan 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

niemeyer left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

github-actions bot commented Oct 17, 2025 •

edited

Loading

letFunny Jan 20, 2026 •

edited

Loading