Skip to content

mz992: skip the push on a dryrun - #999

Open
mzihlmann wants to merge 2 commits into
mainfrom
mz992-dryrun-push-panic
Open

mz992: skip the push on a dryrun#999
mzihlmann wants to merge 2 commits into
mainfrom
mz992-dryrun-push-panic

Conversation

@mzihlmann

@mzihlmann mzihlmann commented Aug 10, 2026

Copy link
Copy Markdown
Collaborator

Fixes #992

A dryrun renders the plan and builds nothing, so DoBuild returns a nil image. DoPush only skipped the push for --no-push, so it dereferenced that nil image and the executor panicked right after printing a complete plan. Combining --dryrun with a real destination is the natural way to preview what a build will push, and it was the one combination that crashed. The push is not the only path that dereferenced the image, the digest files, the tarball and the OCI layout do too. The nil image is how DoBuild signals plan-only, so the caller that reads that convention is the one that skips the push, and DoPush asserts a non-nil image instead of quietly tolerating one.

The golden suite passes --dryrun but calls DoBuild directly and never reaches DoPush, which is why nothing caught this. The repro therefore needs a real executor run, and it has no docker counterpart, so it comes with TestKanikoOnly, a suite for scenarios that live inside kaniko itself and have no oracle to diff against. Several existing tests were being shoehorned into docker parity for want of such a home, either carrying diffoci ignores that only existed to make a meaningless comparison pass, or asking /proc/1/exe at build time whether kaniko was the builder so the docker side would pass vacuously. Those move over here and shed the workarounds.

Summary by CodeRabbit

  • Bug Fixes

    • Dry-run executions no longer attempt to push images, preventing related failures.
    • Image push operations now provide a clear error when no image is available.
    • Improved handling of Kaniko-only integration scenarios and dry-run builds.
  • Tests

    • Added coverage for dry-run panic prevention, tar output, and OCI layout generation.
    • Kaniko-only image builds are now tested separately from Docker-based builds.

@coderabbitai

coderabbitai Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

@mzihlmann, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 45 minutes

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 9def364e-8a66-416c-8f40-b659477d249e

📥 Commits

Reviewing files that changed from the base of the PR and between 11b3e96 and c105105.

📒 Files selected for processing (1)
  • cmd/executor/cmd/root.go
📝 Walkthrough

Walkthrough

The executor now skips pushes during dry-run execution and rejects nil images in DoPush. Integration tests classify selected Dockerfiles as Kaniko-only, bypass Docker builds for them, and validate them through parallel Kaniko builds.

Changes

Dry-run and Kaniko validation

Layer / File(s) Summary
Dry-run push guard
cmd/executor/cmd/root.go, pkg/executor/push.go
Dry-run execution skips DoPush. DoPush asserts that its image argument is non-nil.
Kaniko-only fixtures and classification
integration/dockerfiles/*, integration/images.go
The integration builder classifies Kaniko-only Dockerfiles and configures the mz992 dry-run outputs. Related fixture and comparison settings are updated.
Kaniko-only test execution
integration/integration_test.go
Docker build and layer tests skip Kaniko-only cases. TestKanikoOnly builds matching Dockerfiles with Kaniko in parallel.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Suggested labels: bug, tests

Suggested reviewers: 0hlov3, babs

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the main change: skipping image pushes during dry runs.
Description check ✅ Passed The description explains the cause, fix, affected paths, and testing strategy, although checklist items and release notes are not completed.
Linked Issues check ✅ Passed The changes skip pushes for dry runs, preserve plan output, add a nil-image assertion, and provide real executor coverage for issue #992.
Out of Scope Changes check ✅ Passed The integration fixture and test reorganizations support Kaniko-only coverage for the dry-run fix and do not introduce unrelated scope.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch mz992-dryrun-push-panic

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@codecov

codecov Bot commented Aug 10, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 60.00000% with 2 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
cmd/executor/cmd/root.go 50.00% 1 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

@mzihlmann
mzihlmann force-pushed the mz992-dryrun-push-panic branch from d921095 to 70c6840 Compare August 10, 2026 22:15
@mzihlmann
mzihlmann force-pushed the mz992-dryrun-push-panic branch from 70c6840 to 11b3e96 Compare August 10, 2026 22:23
@mzihlmann
mzihlmann marked this pull request as ready for review August 10, 2026 22:26
@mzihlmann
mzihlmann requested review from 0hlov3, BobDu, babs and nejch August 10, 2026 22:26

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
integration/integration_test.go (1)

251-254: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Assert the dry-run plan output.

TestKanikoOnly only checks that BuildKanikoImage returns no error. A regression that exits successfully without rendering the plan would pass this test. Capture the executor output for Dockerfile_test_issue_mz992 and assert the expected plan output.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@integration/integration_test.go` around lines 251 - 254, Update
TestKanikoOnly around BuildKanikoImage to capture the executor output
specifically for Dockerfile_test_issue_mz992, then assert that it contains the
expected dry-run plan output. Retain the existing error assertion while ensuring
a successful return also requires the plan to be rendered.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@integration/integration_test.go`:
- Around line 251-254: Update TestKanikoOnly around BuildKanikoImage to capture
the executor output specifically for Dockerfile_test_issue_mz992, then assert
that it contains the expected dry-run plan output. Retain the existing error
assertion while ensuring a successful return also requires the plan to be
rendered.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: a9120f2b-f242-42e0-ab96-072f7485b868

📥 Commits

Reviewing files that changed from the base of the PR and between e547979 and 11b3e96.

📒 Files selected for processing (6)
  • cmd/executor/cmd/root.go
  • integration/dockerfiles/Dockerfile_test_issue_mz473
  • integration/dockerfiles/Dockerfile_test_issue_mz992
  • integration/images.go
  • integration/integration_test.go
  • pkg/executor/push.go

@mzihlmann mzihlmann added the bug Something isn't working label Aug 11, 2026
Comment thread cmd/executor/cmd/root.go
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

--dryrun panics when a destination is set

2 participants