Conversation
There was a problem hiding this comment.
馃煝 Approval recommended
The scoped implementation preserves existing behavior by default and includes focused coverage for both transaction modes.
Pull request overview
Bootstraps the SLES base userland before installing application RPMs to prevent package scriptlet failures.
Changes:
- Adds configurable separate base-package installation.
- Enables it for SLES 15.
- Tests combined and separate installation ordering.
File summaries
| File | Description |
|---|---|
targets/linux/rpm/suse/sles15.go |
Enables separate bootstrapping for SLES 15. |
targets/linux/rpm/distro/distro.go |
Adds the configuration option. |
targets/linux/rpm/distro/container.go |
Implements ordered installation transactions. |
targets/linux/rpm/distro/container_test.go |
Verifies installation ordering. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 0
- Review effort level: Balanced
馃挕 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
bce23f8 to
337d3e4
Compare
337d3e4 to
566fce8
Compare
|
Can you add an integration test that triggers the case that fails in the old code? |
Install the SLES base userland before the application RPM and its runtime dependency graph while preserving the combined transaction for other RPM targets. Keep application repositories out of the bootstrap transaction and retain base RPM metadata for minimization. Add a BuildKit-backed SLES integration test that reproduces the old combined-transaction failure in the login_defs scriptlet and validates the split installation path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 54dc3b04-c791-421f-8328-18f11568031e Signed-off-by: Dhwani Shah <dhwanishah@microsoft.com>
566fce8 to
fbd5a18
Compare
|
Added in The new SLES integration test uses the Microsoft SLES repository and the same relevant dependency shape as the motivating containerd build ( I verified the regression in both directions:
The test is under |
| "coreutils": {}, | ||
| "grep": {}, | ||
| "libopenssl3": {}, | ||
| "moby-runc": { |
There was a problem hiding this comment.
Can we simplify this to not use moby-runc specifically since that's provided by a different repo.
Just thinking about failure modes here, we are often hit by external infrastructure issues and this introduces extra external infra for this test suite.
There was a problem hiding this comment.
The moby-runc dependency is intentional because the failure depends on the full zypper dependency graph. I tried smaller SLES-only fixtures, but they also passed with the old combined transaction and therefore did not cover the regression. This is the smallest fixture I found that fails on the old code (login_defs cannot load libreadline.so.7) and passes with the split bootstrap. I agree the additional repository is a failure surface; I can replace it if I find a local fixture that demonstrably fails against the old implementation.
|
The issue this was attempting to fix was caused by a package in packages.microsoft.com's SLES15 repo that includes libgcc_s.so.1 which conflicts with the base repo. |
Install the SLES base userland before the application RPM and its runtime dependency graph. This prevents package scriptlets from invoking a partially installed shell while preserving dependency resolution behavior for other RPM targets.
What this PR does / why we need it:
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):None.
Special notes for your reviewer:
The generic split-transaction approach caused the Azure Linux
ca-certs_overrideintegration test to fail because a weakprebuilt-ca-certificatesrecommendation was committed before the application dependency could override it. The split is therefore intentionally scoped to SLES.