Add supplemental package model#1146
Conversation
Introduce SubPackage and SubPackageDependencies types for supplemental package support (subpackages from shared builds). Add Packages field to Target, validation in Spec.Validate(), and full unit test coverage. Signed-off-by: Brian Goff <cpuguy83@gmail.com>
There was a problem hiding this comment.
Pull request overview
Adds the shared target-level model for supplemental packages.
Changes:
- Adds supplemental package metadata, naming, validation, and build-argument expansion.
- Adds retrieval helpers and unit tests.
- Regenerates the JSON schema.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
target.go |
Integrates supplemental packages into targets. |
subpackage.go |
Defines the supplemental package model and helpers. |
subpackage_test.go |
Tests naming, validation, YAML, and accessors. |
load.go |
Validates supplemental package names. |
docs/spec.schema.json |
Documents the generated schema. |
Reject build argument references in supplemental package names and descriptions while preserving substitution for dependency constraints. Signed-off-by: Brian Goff <cpuguy83@gmail.com>
Keep automatic dependency discovery package-scoped, but reject strip control where targets cannot safely apply it independently. Signed-off-by: Brian Goff <cpuguy83@gmail.com>
invidian
left a comment
invidian
left a comment
There was a problem hiding this comment.
Thanks for splitting the PR. That certainly makes it easier to review.
|
|
||
| for key, pkg := range t.Packages { | ||
| if err := pkg.validate(); err != nil { | ||
| errs = append(errs, errors.Wrapf(err, "package %s", key)) |
There was a problem hiding this comment.
nit: could use %q to guard against empty package names producing weird errors.
| return goerrors.Join(errs...) | ||
| } | ||
|
|
||
| // SubPackage defines a supplemental package produced from the same build |
There was a problem hiding this comment.
Maybe it could be defined on top of file, it feels more substantial than SubPackageDependencies.
| return goerrors.Join(errs...) | ||
| } | ||
|
|
||
| func (s *SubPackage) processBuildArgs(lex *shell.Lex, args map[string]string, allowArg func(string) bool) error { |
There was a problem hiding this comment.
Do we have some documentation on build args? Just looking at the code, build args seems to be related to commands which one runs to build the package, but I'm not sure that's the right thing.
There was a problem hiding this comment.
build-args are a docker-ism.
I imagine its just assumed in our code documentation as to what it is. I suppose it could use a formal definition.
| func (s *SubPackage) processBuildArgs(lex *shell.Lex, args map[string]string, allowArg func(string) bool) error { | ||
| var errs []error | ||
|
|
||
| if err := validateNoBuildArgReferences(s.Name); err != nil { |
There was a problem hiding this comment.
I'm pretty sure we are missing a lot of test cases with examples when those validations fail.
There was a problem hiding this comment.
Yeah. On the last push I asked the clanker to remove arg substitution for name and description here (doesn't make sense and complicates other pieces).
It took that to mean "make sure there are no arg substitutions".
I don't think we should be validating this at all.
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| t.Parallel() | ||
| err := tt.pkg.validate() |
There was a problem hiding this comment.
Can we test validations at exported code level?
| } | ||
| } | ||
|
|
||
| func TestSubPackageYAMLRoundTrip(t *testing.T) { |
There was a problem hiding this comment.
What does it mean round trip here?
|
|
||
| // GetAllPackageNames returns the resolved names of all packages (primary + | ||
| // supplemental) for the given target. The primary package name is always first. | ||
| func (s *Spec) GetAllPackageNames(targetKey string) []string { |
|
|
||
| // GetSubPackages returns the supplemental packages defined for the given target. | ||
| // Returns nil if the target does not exist or has no supplemental packages. | ||
| func (s *Spec) GetSubPackages(targetKey string) map[string]SubPackage { |
There was a problem hiding this comment.
Can we place exported methods higher up?
And assuming we place this method in this file for modularity, shouldn't we change this signature to GetSubPackages(Spec, string) ... perhaps?
Also, it could be named GetSubPackagesForTarget.
| return value != "" | ||
| } | ||
|
|
||
| func (s *SubPackage) fillDefaults() { |
There was a problem hiding this comment.
That should probably be removed together with calling code.
Expose target package selection as a package-level iterator so callers consume deterministic package metadata without materializing maps. Signed-off-by: Brian Goff <cpuguy83@gmail.com>
Keep subpackage metadata literal, validate package rules through the exported spec boundary, and remove unused package helpers. Signed-off-by: Brian Goff <cpuguy83@gmail.com>
Explain why subpackages share the root artifact model while rejecting the sole unsupported strip control at validation time. Signed-off-by: Brian Goff <cpuguy83@gmail.com>
|
Comments addressed. |
Adds target-level supplemental package metadata, validation, build argument expansion, defaults, and generated schema support.
This is layer 1 of the native supplemental-packages stack and provides the shared model used by later target implementations.
Related to #607.