Skip to content

imagebuildah.StageExecutor.Execute: commit more "no instructions" cases#6314

Merged
openshift-merge-bot[bot] merged 1 commit intocontainers:mainfrom
nalind:no-more-instructions
Aug 7, 2025
Merged

imagebuildah.StageExecutor.Execute: commit more "no instructions" cases#6314
openshift-merge-bot[bot] merged 1 commit intocontainers:mainfrom
nalind:no-more-instructions

Conversation

@nalind
Copy link
Member

@nalind nalind commented Aug 4, 2025

What type of PR is this?

/kind bug

What this PR does / why we need it:

When there are no instructions to process, we try to reuse the base image. When we've been told, out of band, to remove labels or environment variables, or affect annotations, we still need to, though, so check for values of more of those flags.

How to verify it

New integration test!

Which issue(s) this PR fixes:

None

Special notes for your reviewer:

Does this PR introduce a user-facing change?

None

When there are no instructions to process, we try to reuse the base
image.  When we've been told, out of band, to remove labels or
environment variables, or affect annotations, we still need to, though,
so check for values of more of those flags.

Signed-off-by: Nalin Dahyabhai <[email protected]>
@openshift-ci openshift-ci bot added kind/bug Categorizes issue or PR as related to a bug. approved labels Aug 4, 2025
if len(children) == 0 {
// There are no steps.
if s.builder.FromImageID == "" || s.executor.squash || s.executor.confidentialWorkload.Convert || len(s.executor.labels) > 0 || len(s.executor.annotations) > 0 || len(s.executor.unsetEnvs) > 0 || len(s.executor.unsetLabels) > 0 || len(s.executor.sbomScanOptions) > 0 || len(s.executor.unsetAnnotations) > 0 {
if s.builder.FromImageID == "" || s.executor.squash || s.executor.confidentialWorkload.Convert || len(s.executor.annotations) > 0 || len(s.executor.unsetEnvs) > 0 || len(s.executor.unsetLabels) > 0 || len(s.executor.sbomScanOptions) > 0 || len(s.executor.unsetAnnotations) > 0 || s.executor.inheritLabels == types.OptionalBoolFalse || s.executor.inheritAnnotations == types.OptionalBoolFalse {
Copy link
Member

Choose a reason for hiding this comment

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

Did you mean to remove:
len(s.executor.labels) > 0 ||

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. With --label and --env, which have corresponding instructions, we append them to the parsed version of the Dockerfile before, making children nonzero in length, so that condition could never be true.

@TomSweeneyRedHat
Copy link
Member

LGTM

Copy link
Collaborator

@flouthoc flouthoc left a comment

Choose a reason for hiding this comment

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

LGTM
/approve
/lgtm

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 7, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: flouthoc, nalind

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-bot openshift-merge-bot bot merged commit 3ed8ff9 into containers:main Aug 7, 2025
37 checks passed
@nalind nalind deleted the no-more-instructions branch August 7, 2025 18:51
@stale-locking-app stale-locking-app bot locked as resolved and limited conversation to collaborators Nov 6, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

approved kind/bug Categorizes issue or PR as related to a bug. lgtm locked - please file new issue/PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants