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.

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

API events: fix parsing error #7088

Merged
merged 2 commits into from
Jul 27, 2020
Merged

API events: fix parsing error #7088

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5ee35ca
API events: fix parsing error
vrothberg Jul 27, 2020
724a09e
test/apiv2: add a simple events test
vrothberg Jul 27, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion pkg/api/handlers/compat/events.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,14 @@ func filtersFromRequest(r *http.Request) ([]string, error) {
compatFilters map[string]map[string]bool
filters map[string][]string
libpodFilters []string
raw []byte
)
raw := []byte(r.Form.Get("filters"))

if _, found := r.URL.Query()["filters"]; found {
raw = []byte(r.Form.Get("filters"))
} else {
return []string{}, nil
}

// Backwards compat with older versions of Docker.
if err := json.Unmarshal(raw, &compatFilters); err == nil {
Expand Down
4 changes: 4 additions & 0 deletions test/apiv2/01-basic.at
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,4 +68,8 @@ else
_show_ok 0 "Time for ten /info requests" "<= 5 seconds" "$delta_t seconds"
fi

# Simple events test (see #7078)
t GET "events?stream=false" 200
t GET "libpod/events?stream=false" 200

Copy link
Member

Choose a reason for hiding this comment

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

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]\\+

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, ?stream=false would be a good candidate for the tests here 👍

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

# vim: filetype=sh