feat(schema): update v1beta1 types#4704
Conversation
Signed-off-by: Austin Abro <austinabro321@gmail.com>
Signed-off-by: Austin Abro <austinabro321@gmail.com>
Signed-off-by: Austin Abro <austinabro321@gmail.com>
Signed-off-by: Austin Abro <austinabro321@gmail.com>
✅ Deploy Preview for zarf-docs canceled.
|
Codecov Report❌ Patch coverage is
... and 9 files with indirect coverage changes 🚀 New features to boost your workflow:
|
Signed-off-by: Austin Abro <austinabro321@gmail.com>
Signed-off-by: Austin Abro <austinabro321@gmail.com>
Signed-off-by: Austin Abro <austinabro321@gmail.com>
Signed-off-by: Austin Abro <austinabro321@gmail.com>
Signed-off-by: Austin Abro <austinabro321@gmail.com>
Signed-off-by: Austin Abro <austinabro321@gmail.com>
Signed-off-by: Austin Abro <austinabro321@gmail.com>
Signed-off-by: Austin Abro <austinabro321@gmail.com>
Signed-off-by: Austin Abro <austinabro321@gmail.com>
Signed-off-by: Austin Abro <austinabro321@gmail.com>
Signed-off-by: Austin Abro <austinabro321@gmail.com>
Signed-off-by: Austin Abro <austinabro321@gmail.com>
Signed-off-by: Austin Abro <austinabro321@gmail.com>
Signed-off-by: Austin Abro <austinabro321@gmail.com>
soltysh
left a comment
There was a problem hiding this comment.
Some high-level comments, but I'll do another pass regarding the API as a whole.
| fmt.Println(err.Error()) | ||
| os.Exit(1) | ||
| } | ||
| } |
There was a problem hiding this comment.
Any particular reason for generating two files for v1beta1? I think from a user perspective, it's easier to import a single file containing all the elements of the schema, rather than multiple.
There was a problem hiding this comment.
There are two files for the two different kinds. One is for the component config kind, while the other is for the package kind. To your point, perhaps we should call one of the package schema and one the component schema
| } | ||
|
|
||
| // GetDeprecatedDataInjections returns the v1alpha1 data injections carried as a backwards-compatibility shim. | ||
| func (c Component) GetDeprecatedDataInjections() []v1alpha1.ZarfDataInjection { |
There was a problem hiding this comment.
I know what you're trying to achieve, but I don't think this is the right way to do it. Currently, you're hiding the field as private, but you're providing public method for setting/getting it. This might mean, external consumers will be able to change it, and that's not what we want. This field should only be settable by our conversion logic, right?
There was a problem hiding this comment.
This comment applies to all private fields here and in other files below.
There was a problem hiding this comment.
Removed the setter, I believe we'll be able to use a different strategy in #4823 that we discussed
There was a problem hiding this comment.
#4823 is in draft, but I'm happy with the strategy there. It exposes a function SetDeprecatedFromGeneric(g types.Package, pkg Package) Package but it's only callable from Zarf since types.Package is internal
| } | ||
|
|
||
| // GetImages returns all image names specified in the component, including those from ImageArchives. | ||
| func (c Component) GetImages() []string { |
There was a problem hiding this comment.
I wonder about these struct methods vs just regular helpers, I don't have any strong preferences, but I like having clean API types only 😅
There was a problem hiding this comment.
I like the ergonomics of pulling data directly from the type, but I see where you're coming from
| // ServerSideApplyDisabled always uses client-side apply. | ||
| ServerSideApplyDisabled ServerSideApplyMode = "false" | ||
| // ServerSideApplyAuto uses server-side apply for fresh installs and matches the prior strategy on upgrade. | ||
| ServerSideApplyAuto ServerSideApplyMode = "auto" |
There was a problem hiding this comment.
Suggestion, the above values, especially true and false might cause problems during yaml conversions. Should we consider turning all constants into upper cases values? Here, I'd specifically use Enabled, Disabled and Auto (alternatively Always, Never and Auto).
There was a problem hiding this comment.
Similarly, above we'd use Registry, SeedRegistry, Injector, etc.
There was a problem hiding this comment.
Since they will be in quotes it should be fine, we haven't run into any issues yet, and this is an existing field in the v1alpha1 schema. I want to keep it as "true", "false", "auto" since this is essentially a pass through of the options on the "helm install --server-side` flag.
| // Default timeout in seconds for commands (default 0, no timeout). | ||
| MaxTotalSeconds int32 `json:"maxTotalSeconds,omitempty"` | ||
| // Retry commands a given number of times if they fail (default 0). | ||
| Retries int32 `json:"retries,omitempty"` |
There was a problem hiding this comment.
I think, for both of these fields, I'm inclined to drop the omitempty, such that those will always show up in the yaml after zarf will process these. This will ensure users have those values explicit, especially when we default them to 0.
There was a problem hiding this comment.
I would disagree there, since removing omitempty will make it so the fields are required by our jsonschema when .actions.defaults is defined
| // The timestamp when this package was created. | ||
| Timestamp string `json:"timestamp"` | ||
| // The version of Zarf used to build this package. | ||
| Version string `json:"version"` |
There was a problem hiding this comment.
Since BuildData field is omitempty inside Package, so there are some edge-cases when these values might show up as "", when at least one of the other fields is set.
There was a problem hiding this comment.
I believe this is okay. Zarf populates these fields, and we treat omitEmpty as a way to say these fields are "required" and Zarf will always populate them out when creating a package.
Signed-off-by: Austin Abro <austinabro321@gmail.com>
Signed-off-by: Austin Abro <austinabro321@gmail.com>
Signed-off-by: Austin Abro <austinabro321@gmail.com>
Signed-off-by: Austin Abro <austinabro321@gmail.com>
Signed-off-by: Austin Abro <austinabro321@gmail.com>
Description
This brings the v1beta1 schema in line with what is proposed in zarf-dev/proposals#52
Relates to #3433
Checklist before merging