Prepare tests and implementation for adding bootstrapped deb containers implementation#968
Conversation
f2a750c to
9acc38e
Compare
There was a problem hiding this comment.
Pull request overview
This PR refactors the Debian container building implementation to prepare for adding bootstrapped (distroless) container support. The changes separate concerns by extracting reusable helper functions and adjust test targets to focus on package building rather than container building where appropriate.
Changes:
- Refactored
targets/linux/deb/distro/container.gointo smaller, more focused functions (baseImageFromSpec,basePackages,extraRepos,installPackagesInContainer) with a sharedbuildContainerInputstruct for passing context - Changed multiple test cases from using
Containertarget toPackagetarget to better isolate package-specific behavior and prepare for multiple container implementation variants - Added optional
source_policyworkflow dispatch input to CI workflow for testing BuildKit source policy functionality
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| test/multiple_artifacts_test.go | Changed artifact tests to use Package target instead of Container target |
| test/linux_target_test.go | Systematically changed tests to use Package target; improved cache ignore test diagnostics with JSON output of ops |
| test/custom_repo_test.go | Changed custom repo tests to Package target and removed unnecessary withPlatformPtr calls |
| targets/linux/deb/distro/container.go | Refactored BuildContainer into smaller, reusable functions with buildContainerInput struct for better modularity |
| .github/workflows/ci.yml | Added optional source_policy input to workflow_dispatch trigger with conditional execution |
Comments suppressed due to low confidence (1)
test/linux_target_test.go:4331
- Both the "package" and "container" test subtests are using
cfg.Target.Packageas the build target. This appears to be unintentional. The "container" subtest should likely usecfg.Target.Containerto properly test container-specific behavior, unless the intention is to remove container-specific testing entirely for this test case.
If the goal is to test both package and container targets with the same failing tests, the current implementation is correct but the subtest name "container" is misleading since it's actually testing the package target.
t.Run("container", testForTarget(cfg.Target.Package))
That should make tests more targetted for their intended scope. Signed-off-by: Mateusz Gozdek <mgozdek@microsoft.com>
9acc38e to
f6a1367
Compare
cpuguy83
left a comment
There was a problem hiding this comment.
LGTM, just some minor comments--maybe utilizing llb.StateOption would be good for some these new functions (and maybe bind them on the buildContainerInput type?
| return input.Config.RepoMounts(repos, input.SOpt, input.Opts...) | ||
| } | ||
|
|
||
| func installPackagesInContainer(baseImg llb.State, input buildContainerInput, ro []llb.RunOption) llb.State { |
There was a problem hiding this comment.
Maybe make this an llb.StateOption?
There was a problem hiding this comment.
Might actually be worth binding these as methods on buildContainerInput.
There was a problem hiding this comment.
Maybe make this an llb.StateOption?
Sure, that make perfect sense.
Might actually be worth binding these as methods on buildContainerInput.
I'd skip that if you don't mind. I think binding a method complects data structure with behaviour, so it yields higher complexity without justification (since we don't deal with e.g. state here like with e.g. with stack implementation), so I'd rather keep it separate. Of course it's a small and not very important piece of code, but I think it still contributes to spreading patterns for keeping the complexity low where possible.
…ainers Signed-off-by: Mateusz Gozdek <mgozdek@microsoft.com>
f6a1367 to
b2a1b11
Compare
What this PR does / why we need it:
This PR refactors existing implementation of building deb containers into smaller functions which will be re-used by the upcoming bootstrapped containers implementation.
It also changes some tests to target package instead of container, so we can better confine container-specific tests and make them re-usable across different targets, namely existing deb testing target and upcoming new minimal containers.
Which issue(s) this PR fixes (optional, using
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when the PR gets merged):Part of #448
Special notes for your reviewer:
I am not sure whether change of targets reduce our test coverage in some way.