Consider Switching to type aliasing to reduce code duplication

[prestist]

Type aliasing suggestion

While evaluating an issue in Ignition, I noticed the tendency to duplicate the schema's for each version. This was done to allow for adjusting a schema's individual attributes separately per schema version. However, it seems that in practice defined structs do not change per schema version, and tend to stay in place with a few exceptions. Furthermore with a larger and larger list of schemas we find ourselves duplicating patches that go into each schema, and then having to duplicate the unit tests for those changes. You can see a working example of this problem here.

Switching to type aliasing can help us reduce duplication and should reduce growing pains as we stabilize more schemas. Once a type is defined, future schema's can then build a type alias from it, and if need be can then expand its functionality for that schema forward. This should then reduce the amount of redundant tests and struct definitions per schema.

Looking at the fcos 1_4 schema.go, we have 4 structs, all of which are present in the previous schema.go 1_3.

So as a simple example of what aliasing can do, instead of having:

package v1_4

import (
	base "github.com/coreos/butane/base/v0_4"
)

type Config struct {
	base.Config `yaml:",inline"`
	BootDevice  BootDevice `yaml:"boot_device"`
}

type BootDevice struct {
	Layout *string          `yaml:"layout"`
	Luks   BootDeviceLuks   `yaml:"luks"`
	Mirror BootDeviceMirror `yaml:"mirror"`
}

type BootDeviceLuks struct {
	Tang      []base.Tang `yaml:"tang"`
	Threshold *int        `yaml:"threshold"`
	Tpm2      *bool       `yaml:"tpm2"`
}

type BootDeviceMirror struct {
	Devices []string `yaml:"devices"`
}

We could have:

package v1_4

import (
	"github.com/coreos/butane/config/fcos/v1_3"
)

type Config v1_3.Config

type BootDevice v1_3.BootDevice

type BootDeviceLuks v1_3.BootDeviceLuks

type BootDeviceMirror v1_3.BootDeviceMirror

Showing a more complicated example would be the transition from 1_4 to 1_5_exp.

package v1_5_exp

import (
	base "github.com/coreos/butane/base/v0_5_exp"
)

type Config struct {
	base.Config `yaml:",inline"`
	BootDevice  BootDevice  `yaml:"boot_device"`
	Extensions  []Extension `yaml:"extensions"`
	Grub        Grub        `yaml:"grub"`
}

type BootDevice struct {
	Layout *string          `yaml:"layout"`
	Luks   BootDeviceLuks   `yaml:"luks"`
	Mirror BootDeviceMirror `yaml:"mirror"`
}

type BootDeviceLuks struct {
	Tang      []base.Tang `yaml:"tang"`
	Threshold *int        `yaml:"threshold"`
	Tpm2      *bool       `yaml:"tpm2"`
}

type BootDeviceMirror struct {
	Devices []string `yaml:"devices"`
}

type Extension struct {
	Name string `yaml:"name"`
}

type Grub struct {
	Users []GrubUser `yaml:"users"`
}

type GrubUser struct {
	Name         string  `yaml:"name"`
	PasswordHash *string `yaml:"password_hash"`
}

This should result in

package v1_5_exp

import (
        base "github.com/coreos/butane/base/v0_5_exp"
	"github.com/coreos/butane/config/fcos/v1_3"
)

type Config struct {
	base.Config `yaml:",inline"`
	BootDevice  BootDevice  `yaml:"boot_device"`
	Extensions  []Extension `yaml:"extensions"`
	Grub        Grub        `yaml:"grub"`
}

type BootDevice v1_3.BootDevice

type BootDeviceLuks v1_3.BootDeviceLuks

type BootDeviceMirror v1_3.BootDeviceMirror

type Extension struct {
	Name string `yaml:"name"`
}

type Grub struct {
	Users []GrubUser `yaml:"users"`
}

type GrubUser struct {
	Name         string  `yaml:"name"`
	PasswordHash *string `yaml:"password_hash"`
}

My proposal for the existing tests after implementing type aliasing:

• We should remove existing duplicated tests, the test that cover unchanged functionality from one schema to the next. In the case a type's functionality/attributes are expanded between schemas then, it should then have all existing tests ran on the new and improved type.

Impact on stabilizing spec

The deduplication doesn't just stop at tests, it should also reduce the impact for bumping the spec versions as well. In reading you can tell the process is burdened by the fact that every spec is completely its own silo. In introducing type aliasing we should be able to reduce this process. Rather than copying the now "stabilized" spec into an experimental folder, we would create a new spec that references the "stabilized" types. Then since there would initially be no new functionality the tests should not need to be carried over.

I am new to this project, but I thought it would be good to get everyones thoughts?

[bgilbert]

This seems plausible to me, though I may have missed something.

The advantage of the current spec stabilization process is that it's well-documented, mostly mechanical, and doesn't require a lot of special knowledge. I think the proposed stabilization process could achieve that as well, but we should try to make sure that we're not introducing a bunch of underspecified process that turns out to be harder than it sounds.

To be clear, your proposal wouldn't work well for Ignition because Ignition's types are generated from JSON Schema by schematyper. Butane doesn't have that restriction, and also has more spec versions.