-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
podman events
: check for an error after we finish reading events
#22691
podman events
: check for an error after we finish reading events
#22691
Conversation
Ephemeral COPR build failed. @containers/packit-build please check. |
LGTM |
cmd/podman/system/events.go
Outdated
if err != nil { | ||
return err | ||
} | ||
case <-time.After(1 * time.Millisecond): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assuming you did this to get a non blocking call on the error channel you should use the default
case instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either one assumes that the error will be ready by the time the select
statement looks at the channel, but yeah, default
would be cleaner. Changed it.
2abb9cd
to
ace8080
Compare
Cockpit tests failed for commit ace8080. @martinpitt, @jelly, @mvollmer please check. |
I hate the nested select, but I don't see another way. LGTM once you get the test to pass. |
ace8080
to
dc7a0ed
Compare
Ephemeral COPR build failed. @containers/packit-build please check. |
Cockpit tests failed for commit dc7a0ed. @martinpitt, @jelly, @mvollmer please check. |
dc7a0ed
to
e1320a8
Compare
Ephemeral COPR build failed. @containers/packit-build please check. |
pkg/bindings/system/system.go
Outdated
var bodyText string | ||
if body, err := io.ReadAll(response.Body); err == nil { | ||
bodyText = string(body) | ||
} | ||
var errorModel errorhandling.ErrorModel | ||
if err := json.Unmarshal([]byte(bodyText), &errorModel); err == nil && errorModel.Message != "" { | ||
bodyText = errorModel.Message | ||
} | ||
if response.Status != "" { | ||
if bodyText != "" { | ||
return fmt.Errorf("%s: %s", response.Status, bodyText) | ||
} | ||
return errors.New(response.Status) | ||
} | ||
if bodyText != "" { | ||
return fmt.Errorf("server responded with unexpected status code %d: %s", response.StatusCode, bodyText) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the rest of the code simply calls return response.Process(nil)
to convert the error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, that's much better. Thanks!
Cockpit tests failed for commit e1320a8. @martinpitt, @jelly, @mvollmer please check. |
The function that's handing us events will return an error after closing the channel over which it's sending events, and its caller (in its own goroutine) will then send that error over another channel. The logic that started the goroutine is likely to notice that the events channel is closed before noticing that the error channel has a result for it to read, so any error that would have been communicated would be lost. When we finish reading events, check if the reader returned an error before telling our caller that there was no error. Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>
e1320a8
to
c46884a
Compare
Ephemeral COPR build failed. @containers/packit-build please check. |
Cockpit tests failed for commit c46884a. @martinpitt, @jelly, @mvollmer please check. |
Ephemeral COPR build failed. @containers/packit-build please check. |
Cockpit tests failed for commit c46884a. @martinpitt, @jelly, @mvollmer please check. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Luap99, nalind 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 |
759e546
into
containers:main
Does this PR introduce a user-facing change?