Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

stage_executor: allow images without layers #5031

Merged
merged 1 commit into from Sep 24, 2023
Merged

stage_executor: allow images without layers #5031

merged 1 commit into from Sep 24, 2023

Conversation

flouthoc
Copy link
Collaborator

@flouthoc flouthoc commented Sep 7, 2023 •
edited

A build case where base is scratch and nothing else, then produce image without layer.

This matches with buildkit and docker.

What type of PR is this?

/kind api-change
/kind bug
/kind cleanup
/kind deprecation
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake
/kind other

What this PR does / why we need it:

How to verify it

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

 stage_executor: allow images without layers

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 7, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: flouthoc

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

The pull request process is described here

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-ci openshift-ci bot added the approved label Sep 7, 2023
@flouthoc flouthoc mentioned this pull request Sep 7, 2023
Closed
@rhatdan
Copy link
Member

rhatdan commented Sep 7, 2023

LGTM

@TomSweeneyRedHat
Copy link
Member

LGTM
But I would suggest changing the release note to:

When a build uses "scratch" for the image and only a label is added, a layer is now added to the image rather than creating an empty image.

@nalind
Copy link
Member

nalind commented Sep 7, 2023

If an image with no layers is a valid thing, then this is a bug in the logic that lists which images are in storage, and that's where this needs to be fixed. I'd suggest adding error context with fmt.Errorf() in layer_tree.go and going from there.

@flouthoc
Copy link
Collaborator Author

flouthoc commented Sep 8, 2023 •
edited

@nalind SGTM but one thing which bothers me is that when a single FROM scratch is created without any labels it creates a layer, but as soon as a label is added it does not creates any layer in the image. That part looked inconsistent.

If an image with no layers is a valid thing, then this is a bug in the logic

Yes this is which i needed to verify, checking on this.

@flouthoc
Copy link
Collaborator Author

flouthoc commented Sep 8, 2023

Well it seems, docker/buildkit allows creating image without layers so we gotta do that do, and buildkit produces no layer even for FROM scratch , I'll modify this PR to make behavior of FROM scratch consistent with buildkit.

@flouthoc
Copy link
Collaborator Author

flouthoc commented Sep 8, 2023

/hold

@TomSweeneyRedHat
Copy link
Member

@flouthoc looks like you'll need to rebase.

@flouthoc flouthoc changed the title stage_executor: create layer if only empty_layer is present stage_executor: allow images without layers Sep 14, 2023
@flouthoc
Copy link
Collaborator Author

@nalind I have modified this PR , and using this PR to make buildah's behavior consistent with docker and buildkit and meanwhile the original issue , i'm trying to fix that here: containers/common#1651

@flouthoc
Copy link
Collaborator Author

@nalind @containers/buildah-maintainers PTAL original issue containers/podman#19860 is closed in a different PR, following PR brings consistency with buildkit and docker

@flouthoc
stage_executor: allow images without layers
ae5f11e 
`Docker` and `Buildkit` creates no layers when only `FROM scratch` is
used as content for building image, buildah must do the same.

Signed-off-by: Aditya R <arajan@redhat.com>
@nalind
Copy link
Member

nalind commented Sep 22, 2023

LGTM

@rhatdan
Copy link
Member

rhatdan commented Sep 24, 2023

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Sep 24, 2023
@openshift-merge-robot openshift-merge-robot merged commit be13566 into containers:main Sep 24, 2023
36 checks passed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@nalind nalind Awaiting requested review from nalind

Assignees

@rhatdan rhatdan

Labels
approved lgtm locked - please file new issue/PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@flouthoc @rhatdan @TomSweeneyRedHat @nalind @openshift-merge-robot