Skip to content

read stdout for reading container name #745

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
Merged
merged 1 commit into from
Feb 3, 2022
Merged

read stdout for reading container name #745

merged 1 commit into from
Feb 3, 2022

Conversation

@ankitathomas
Copy link
Contributor

@ankitathomas ankitathomas commented Aug 9, 2021

Read container names from only stdout to avoid podman warnings on stderr.

Closes #689

@openshift-ci openshift-ci bot requested review from exdx and timflannagan August 9, 2021 13:24
@codecov
Copy link

codecov bot commented Aug 9, 2021 •
edited
Loading

Codecov Report

Merging #745 (75d254d) into master (c2fc1fd) will increase coverage by 0.23%.
The diff coverage is 50.20%.

❗ Current head 75d254d differs from pull request most recent head 6dc9024. Consider uploading reports for the commit 6dc9024 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #745      +/-   ##
==========================================
+ Coverage   50.19%   50.43%   +0.23%     
==========================================
  Files         100      102       +2     
  Lines        8531     8721     +190     
==========================================
+ Hits         4282     4398     +116     
- Misses       3421     3473      +52     
- Partials      828      850      +22
Impacted Files Coverage Δ
alpha/action/generate_dockerfile.go 81.81% <ø> (ø)
alpha/action/init.go 100.00% <ø> (ø)
alpha/action/list.go 67.70% <0.00%> (ø)
alpha/declcfg/declcfg.go 100.00% <ø> (ø)
alpha/declcfg/declcfg_to_model.go 76.19% <ø> (ø)
alpha/declcfg/load.go 83.56% <ø> (ø)
alpha/declcfg/model_to_declcfg.go 100.00% <ø> (ø)
alpha/declcfg/properties.go 100.00% <ø> (ø)
alpha/declcfg/write.go 71.69% <ø> (ø)
alpha/model/error.go 100.00% <ø> (ø)
... and 27 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c2fc1fd...6dc9024. Read the comment docs.

exdx
exdx reviewed Aug 9, 2021
View reviewed changes
pkg/containertools/runner.go Outdated
if exitErr, ok := err.(*exec.ExitError); ok {
msg = fmt.Sprintf("%s: %s", err, exitErr.Stderr)
}
r.logger.Errorf(msg)
Copy link
Contributor

@exdx exdx Aug 9, 2021

Choose a reason for hiding this comment

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

nit: would logging the error here and then bubbling it up cause the message to occur twice in a row in the command output?

Copy link
Member

@timflannagan timflannagan Aug 12, 2021

Choose a reason for hiding this comment

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

Yeah, generally speaking, logging at the source and then returning an error can lead to duplicate error logging, so if we can double-check how we're handling logging at the various call sites, and instead bubble the error up that should be sufficient.

Copy link
Contributor Author

@ankitathomas ankitathomas Sep 1, 2021

Choose a reason for hiding this comment

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

Looks like it bubbles up, fixed the extra logging

dubee
dubee suggested changes Aug 16, 2021
View reviewed changes
Copy link

@dubee dubee left a comment

Choose a reason for hiding this comment

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

@ankitathomas, can you also change the other occurrences of CombinedOutput in the Unpack method? It looks like there are two more calls in Unpack that combine stdout and stderr.

dubee
dubee reviewed Aug 31, 2021
View reviewed changes
Copy link

@dubee dubee left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@ankitathomas, any updates on this one?

@ankitathomas
use only stdout for reading container name
6dc9024 
Signed-off-by: Ankita Thomas <ankithom@redhat.com>
@ankitathomas
Copy link
Contributor Author

ankitathomas commented Sep 1, 2021

@ankitathomas, can you also change the other occurrences of CombinedOutput in the Unpack method? It looks like there are two more calls in Unpack that combine stdout and stderr.

The other uses of CombinedOutput are only ever used as error messages, unlike the one instance fixed here that was using the CombinedOutput to look for a container name on stdout. The issue should go away with just this one fixed.

@timflannagan
Copy link
Member

timflannagan commented Jan 5, 2022

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 5, 2022
@joelanford
Copy link
Member

joelanford commented Feb 3, 2022

/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Feb 3, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ankitathomas, joelanford

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 Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 3, 2022
@openshift-merge-robot openshift-merge-robot merged commit d59081d into operator-framework:master Feb 3, 2022
@exdx exdx mentioned this pull request Feb 7, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@joelanford joelanford Awaiting requested review from joelanford

@kevinrizza kevinrizza Awaiting requested review from kevinrizza

@dinhxuanvu dinhxuanvu Awaiting requested review from dinhxuanvu

@timflannagan timflannagan Awaiting requested review from timflannagan

2 more reviewers

@dubee dubee dubee requested changes

@exdx exdx exdx left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@timflannagan timflannagan

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

OPM fails to create index with Podman 3.0.1

6 participants

@ankitathomas @timflannagan @joelanford @dubee @exdx @openshift-merge-robot