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

Plumb the remote logger throughut Buildah #3540

Merged
merged 1 commit into from
Sep 30, 2021

Conversation

rhatdan
Copy link
Member

@rhatdan rhatdan commented Sep 24, 2021

Users are not seeing Warnings when doing podman --remote build.
We need to wire the logrus messages all the way through the system.

Fixes: #3537

Signed-off-by: Daniel J Walsh dwalsh@redhat.com

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?


@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 24, 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

@TomSweeneyRedHat
Copy link
Member

@rhatdan lint is barking

@rhatdan rhatdan force-pushed the log-level branch 6 times, most recently from 8583a19 to a4184ed Compare September 28, 2021 00:23
@rhatdan
Copy link
Member Author

rhatdan commented Sep 28, 2021

config.go Outdated
@@ -545,7 +545,7 @@ func (b *Builder) Comment() string {
// discarded when writing images using OCIv1 formats.
func (b *Builder) SetComment(comment string) {
if comment != "" && b.Format != define.Dockerv2ImageManifest {
logrus.Warnf("COMMENT is not supported for OCI image format, comment %s will be ignored. Must use `docker` format", comment)
b.Logger.Warnf("COMMENT is not supported for OCI image format, comment %s will be ignored. Must use `docker` format", comment)
Copy link
Member

Choose a reason for hiding this comment

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

COMMENT is not an instruction, so this message is misleading.

Copy link
Member Author

Choose a reason for hiding this comment

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

What should the message be? Or is this actually impossible?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok since this is only called via buildah config, I will just remove the change. It does not look like buildah build or podman-remote build would ever hit this code.

config.go Outdated
@@ -600,7 +600,7 @@ func (b *Builder) SetHealthcheck(config *docker.HealthConfig) {
b.Docker.Config.Healthcheck = nil
if config != nil {
if b.Format != define.Dockerv2ImageManifest {
logrus.Warnf("Healthcheck is not supported for OCI image format and will be ignored. Must use `docker` format")
b.Logger.Warnf("Healthcheck is not supported for OCI image format and will be ignored. Must use `docker` format")
Copy link
Member

Choose a reason for hiding this comment

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

Might as well upper-case the HEALTHCHECK here.

@TomSweeneyRedHat
Copy link
Member

@rhatdan, might need a rebase.
LGTM other than @nalind 's comments

Users are not seeing Warnings when doing podman --remote build.
We need to wire the logrus messages all the way through the system.

Fixes: containers#3537

Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>
@umohnani8
Copy link
Member

LGTM
@rhatdan test seems unhappy though

@rhatdan rhatdan added the lgtm label Sep 30, 2021
@openshift-merge-robot openshift-merge-robot merged commit bc718ca into containers:main Sep 30, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 15, 2023
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.

Dockerfile SHELL instruction seems to be ignored
5 participants