-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Match output of Compat Top API to Docker #23986
Conversation
We were only splitting on tabs, not spaces, so we returned just a single line most of the time, not an array of the fields in the output of `ps`. Unfortunately, some of these fields are allowed to contain spaces themselves, which makes things complicated, but we got lucky in that Docker took the simplest possible solution and just assumed that only one field would contain spaces and it would always be the last one, which is easy enough to duplicate on our end. Fixes containers#23981 Signed-off-by: Matt Heon <[email protected]>
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, this looks to simple for me but I guess if tests are passing that should be fine
CTRNAME=testtopproc | ||
podman run --name $CTRNAME -d $IMAGE sleep 25 | ||
t GET containers/$CTRNAME/top?stream=false 200 \ | ||
.Processes.[0].[6]="00:00:00" \ | ||
.Processes.[0].[7]="sleep 25" | ||
podman rm -f -t0 $CTRNAME |
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.
I guess you could add the output check to the top call above? Any reason to run another container for this?
I mean speed is not that big of a deal in API tests so I can live with that and not block over this.
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.
We want a multi-part command - top
doesn't actually test the combining code as it doesn't have a space in it.
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.
ah yes of course
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Luap99, mheon 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 |
Tests green |
/lgtm |
We were only splitting on tabs, not spaces, so we returned just a single line most of the time, not an array of the fields in the output of
ps
. Unfortunately, some of these fields are allowed to contain spaces themselves, which makes things complicated, but we got lucky in that Docker took the simplest possible solution and just assumed that only one field would contain spaces and it would always be the last one, which is easy enough to duplicate on our end.Fixes #23981
Does this PR introduce a user-facing change?