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.

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

fix podman build -o - #23897

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

Napuu
Copy link

@Napuu Napuu commented Sep 9, 2024

Ensure quiet flag is true when building directly to stdout.

Fixes #23063

Does this PR introduce a user-facing change?

Fixed podman build -o - to prevent status messages from appearing in stdout.
Copy link
Contributor

openshift-ci bot commented Sep 9, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Napuu
Once this PR has been reviewed and has the lgtm label, please assign lsm5 for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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

Copy link

Ephemeral COPR build failed. @containers/packit-build please check.

@rhatdan
Copy link
Member

rhatdan commented Sep 9, 2024

This looks like it should be handled in the build options CLI?

https://github.com/containers/buildah/blob/main/pkg/cli/build.go#L267-L275

Copy link
Member

@edsantiago edsantiago left a comment

Choose a reason for hiding this comment

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

Thank you for your PR! LGTM on first (quick) pass, but one blocker.

test/e2e/build_test.go Outdated Show resolved Hide resolved
@lsm5
Copy link
Member

lsm5 commented Sep 10, 2024

The failed packit jobs for epel-9 and fedora-39 will go away after a rebase on main.

Ensure quiet flag is true when building directly to stdout.

Fixes containers#23063

Signed-off-by: Santeri Kääriäinen <santeri.kaariainen@iki.fi>
@edsantiago
Copy link
Member

Test LGTM. Did you look into Dan's question? I think we should try to understand why the pkg/cli code is not having the desired effect, and fix it there. Would you be able to investigate that?

Also, for when you repush, these might be two good additions to your test:

		Expect(output).ShouldNot(ContainSubstring("STEP "))
		Expect(output).ShouldNot(ContainSubstring("COPY "))
@Napuu
Copy link
Author

Napuu commented Sep 12, 2024

The mentioned code in Buildah isn't having the desired effect because that function is not used by Podman. A somewhat similar functionality is implemented in the function where I made my changes. Seems like it's already acknowledged that it shouldn't really be on Podman's side: https://github.com/containers/podman/blob/main/cmd/podman/common/build.go#L270

I think that would need a bigger refactoring to get rid of that duplicated part altogether.

@rhatdan
Copy link
Member

rhatdan commented Sep 12, 2024

Can you change your PR to use said code in Buildah?

@Napuu
Copy link
Author

Napuu commented Sep 12, 2024

Did some initial digging, and it doesn't seem as trivial as my previous change, so not for now at least.

I'll use buildah build -o - to achieve this for the time being. Might have time to investigate more in the future though.

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 7, 2024
@openshift-merge-robot
Copy link
Collaborator

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. release-note
5 participants