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

Wire logger through to config #3663

Merged
merged 2 commits into from
Dec 3, 2021
Merged

Conversation

rhatdan
Copy link
Member

@rhatdan rhatdan commented Dec 3, 2021

Currently we are only wiring the logger into run_linux.go
Not into the Config section.

This PR is needed in order to update vendor in Podman.
containers/podman#12375

[NO NEW TESTS NEEDED] Tests will be done in Podman.

Signed-off-by: Daniel J Walsh [email protected]

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?


Currently we are only wiring the logger into run_linux.go
Not into the Config section.

This PR is needed in order to update vendor in Podman.
containers/podman#12375

[NO NEW TESTS NEEDED] Tests will be done in Podman.

Signed-off-by: Daniel J Walsh <[email protected]>
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 3, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rhatdan

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 Dec 3, 2021
@rhatdan
Copy link
Member Author

rhatdan commented Dec 3, 2021

@nalind
Copy link
Member

nalind commented Dec 3, 2021

Could use a rebase. LGTM pending CI.

@TomSweeneyRedHat
Copy link
Member

LGTM
once tests are happy and a rebase is done.

@nalind
Copy link
Member

nalind commented Dec 3, 2021

I don't really know how podman's using this, but this looks like it should be adding a Logger field to ImportOptions so that we can do the right thing for builders that were created by prior processes.

@flouthoc
Copy link
Collaborator

flouthoc commented Dec 3, 2021

Do we need to instrument code further to use Logger in buildah code. But i guess that could be a different PR.

@flouthoc
Copy link
Collaborator

flouthoc commented Dec 3, 2021

@rhatdan Where are we going to use this field in podman.

@rhatdan
Copy link
Member Author

rhatdan commented Dec 3, 2021

This returns logger messages from buildah like.

$ podman --remote  build --no-cache /tmp/test/
STEP 1/2: FROM fedora
STEP 2/2: SHELL [ "/bin/sh" ]
COMMIT
WARN[0000] SHELL is not supported for OCI image format, [/bin/sh] will be ignored. Must use `docker` format 
--> c8a66c46f2e
c8a66c46f2e29c557d22e98700b528a18adb69fef39d25540a90312619e7171c
podman (hosts) $ 

Without this change you see:
WARN[0000] SHELL is not supported for OCI image format, [/bin/sh] will be ignored. Must use docker format
On the server side not on the client side. And the user is left to wonder why it does not work.

@flouthoc
Copy link
Collaborator

flouthoc commented Dec 3, 2021

LGTM
ah makes sense

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants