ZEP-0051: v1beta1 schema

[AustinAbro321]

• One-line PR description: This proposes the v1beta1 schema

• Issue link: v1beta1 schema #51

[soltysh]

Have a look at a rough sketch where I've implemented some of the comments I've mentioned above, but also a few others: https://gist.github.com/soltysh/eaab0ac6a0113af60c3ce1013b85cfc7

[soltysh]

1. Move Uncompressed, Architecture and AllowNamespaceOverride to package definition.
2. Why do we need Architecture here, if we already specify that on a per-component target? It seems confusing. If this is not binding and I believe it's not, but more like a suggestion, I'd push to either placing that in annotations or documentation.

[AustinAbro321]

1. I'd rather keep them in metadata, as I think it provides a more clear UX, to have all these fields in the same place
2. Architecture is binding as it decides the platforms of the images are pulled and which components are built. Note that architecture can be set from .metadata.architecture but can also be set from the command line (e.g. zarf package create -a arm64). So .component.target.architecture is dependent on .metdata.architecture, when component.target.architecture is set, the component is only built if that architecture matches .metadata.architecture

[soltysh]

I assume the priorities for architecture are as follows:

1. user provided with -a
2. .metadata.architecture
3. everything

?

[AustinAbro321]

Yup

[soltysh]

From json marshalling pov there's no difference between empty value and false value. Those are considered identical. This means that whatever the user sets explicitly here it might get lost. Iow. both: Uncompressed=false and unset will marshal into empty field b/c of omitempty. Is that the intention here and with all of the rest of the bool fields? Or do we want to be explicit about that differentiation?

[AustinAbro321]

So with the uncompressed field, it's fine because uncompressed=false is the default, so whether or not a user set uncompressed: false or not the file will be compressed. With fields where the default is not false, we use bool pointers and function on the package struct that gives the default. For instance

func (zc ZarfChart) ShouldRunSchemaValidation() bool {
	if zc.SchemaValidation != nil {
		return *zc.SchemaValidation
	}
	return true
}

Though this is not 100% consistent since some fields that have a default of false also use bool pointers. Do you think we should use bool pointers everywhere, or only use them when boolean fields default to true?

[soltysh]

Only, when we need to differentiate user-set value and not set. Ideally, we should name the fields such that the default value is the type's empty, in case of bool that means false. I believe, with all my proposals that should be true.

[soltysh]

Can you walk me through the various fields here? I feel like I'm a bit confused when source is directory or remote URL and how this plays with the following:

• Symlinks, why this is a list where others are just a single fields
• ExtractPath, what is that? and partially also how this relates to not being a list?
• Template, is it needed here?

[soltysh]

If, we're handling a single file it all makes sense to me, but once we start talking about input being a directory, then the mixture of lists (Symlinks) vs single value fields (Executable, ExtractPath) is confusing.

[AustinAbro321]

• Symlinks is a list of symlinks that will be created on the host machine during zarf package deploy. symlinks works for both files and directories
• extract path only works if the file is an archive (tar/zip/any). If it is then Zarf will look inside the archive and only extract what is at the extract path to the target destination.
• template decides whether or not we go template the file.

To me a list of symlinks makes sense since a single file or directory can be symlinked to multiple times. Executable makes either the file executable or every file in the directory executable. There is some weirdness in that extractPath only works on archived files.

[soltysh]

To me a list of symlinks makes sense since a single file or directory can be symlinked to multiple times.

I can argue both ways, honestly 😅 Do we have usage of it anywhere?

Executable makes either the file executable or every file in the directory executable.

Honestly, that makes me a bit worried. This potentially can make more files executable than necessary. Especially when you're working with an archive. It makes sense to make it work with a single file, but with archive I don't think I'd feel comfortable doing so. Alternatively we should also provide a list, but that's an overkill.

There is some weirdness in that extractPath only works on archived files.

Actually, this one was the most understandable one for me 😉

Thinking more, should we split File somehow to work with per-file and with an archive, maybe? This could strengthen the posture of that definition. Mixing the two is problematic, to say the least.

[soltysh]

Also, regarding Template, if this is the same Template as before, which enables values, I'd also rename that accordingly.

[AustinAbro321]

Do we have usage of it anywhere

Yeah an example can be seen with k3s, where we symlink k3s since k3s looks at the binary name to determine if it should become a different tool that is embedded. See https://github.com/zarf-dev/zarf/blob/main/packages/distros/k3s/zarf.yaml#L39-L42.

This potentially can make more files executable than necessary.

True, but since users could as easily add another entry to the File struct to mark a single file as executable I'd rather keep the behavior as intuitive as possible and let the user decide.

should we split File somehow to work with per-file and with an archive

This would probably be cleaner, however I think I would rather limit the surface area files considering it's more of a backdoor to allow users to provide files to package kubernetes distributions. A strong majority of packages don't use it, and I'd imagine even less use extractPath so imo the juice isn't worth the squeeze.

[soltysh]

I believe the few things that I'd like to see addressed are:

1. PackageMetadata + ComponentMetadata (either of the variants proposed in ZEP-0051: v1beta1 schema #52 (comment))
2. Review all bool fields, to ensure we have false being the default for all, and the name implies that fact as well.
3. For File should we split archives to be a separate struct? (see ZEP-0051: v1beta1 schema #52 (comment))
4. Template to rename to EnableValues for Manifest and File, also what Template stands for in ComponentAction? Is it different from the other two? I'd like to disambiguate, if they are different.

[AustinAbro321]

@soltysh Yeah everywhere template is used we template the file using go templating with Zarf values. I want to make sure it's clear to users that when they make enableValues: true that their file will be go templated, even if there are no Zarf values, but enableValues is probably clear enough. Another alternative could be goTemplate though enableValues has the advantage of not being implementation specific so I think I'll go with that.

[AustinAbro321]

@soltysh Ready for another review. Resolved most conversations. Didn't split files into separate structs as we haven't gotten much feedback on it, and I'd rather it not be a focal point of the schema. It's a judgement call though