API events: fix parsing error #7088
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vrothberg 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 |
openshift-ci-robot
added
the
approved
Fix an error where an absent "filters" parameter led to JSON parsing errors. Fixes: containers#7078 Signed-off-by: Valentin Rothberg <[email protected]>
|
I am under the impression that the tests in |
|
Added a test the apiv2 test to prevent future regressions. |
Add a simple test to exercise the events API without the "filters" parameter. Prevents regressing on containers#7078. Signed-off-by: Valentin Rothberg <[email protected]>
| # Simple events test (see #7078) | ||
| t GET "events?stream=false" 200 | ||
| t GET "libpod/events?stream=false" 200 | ||
|
|
There was a problem hiding this comment.
Friendly reminder to check actual results, not just status code. From a quick look at the returned JSON, might I suggest:
t GET "events?stream=false" 200 .Type=system .scope=local .time~[0-9]\\+
t GET "libpod/events?stream=false" 200 .Type=system .scope=local .time~[0-9]\\+There was a problem hiding this comment.
Thanks for checking! It doesn't return any data here though. I really just want to exercise this specific code which we can't with bindings as the "filters" param is set unconditionally afaics.
There was a problem hiding this comment.
PS last week I looked into adding tests for /events but gave up because of the follow nature - I didn't want tests to hang. If you have hints for how to write non-hanging tests (looks like ?stream=false is the magic token I was missing) I would like to look into that again. Thank you.
There was a problem hiding this comment.
Yes, ?stream=false would be a good candidate for the tests here 👍
There was a problem hiding this comment.
It doesn't return any data here
Weird! As in: when you run the test I suggested, it fails?? That alarms me a little bit: I tried my tests root and rootless.
There was a problem hiding this comment.
It now succeeds without ?filters=.... It failed before this change with a regression I introduced. The other tests always set "filters", so here I just wanted to make sure that the code path is executed.
|
LGTM |
|
LGTM |
1 similar comment
|
LGTM |
|
/lgtm |
github-actions
bot
added
the
locked - please file new issue/PR
Fix an error where an absent "filters" parameter led to JSON parsing
errors.
Fixes: #7078
Signed-off-by: Valentin Rothberg [email protected]