-
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
Return title fields as a list #18136
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rhatdan 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 aren't happy. Let's not make this block 4.5. |
e83409a
to
8052e70
Compare
Podman is attempting to split the headers returned by the ps command into a list of headers. Problem is that some headers are multi-word, and headers are not guaranteed to be split via a tab. This PR splits the headers bases on white space, and for the select group of CAPS headers which are multi-word, combines them back together. Fixes: containers#17524 Signed-off-by: Daniel J Walsh <[email protected]>
/cherry-pick v4.5 |
@mheon: once the present PR merges, I will cherry-pick it on top of v4.5 in a new PR and assign it to you. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
LGTM |
/lgtm |
@mheon: new pull request created: #18191 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
for _, title := range strings.Fields(output) { | ||
switch title { | ||
case "AMBIENT", "INHERITED", "PERMITTED", "EFFECTIVE", "BOUNDING": | ||
{ | ||
titles = append(titles, title+" CAPS") | ||
} | ||
case "CAPS": | ||
continue | ||
default: | ||
titles = append(titles, title) | ||
} |
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.
Can we somehow expose this in psgo? This looks rather hacky and will break every-time psgo adds a new header with space. cc @vrothberg
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.
This functionality doesn't look being tested. A flip in psgo
wouldn't hurt.
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.
Are we guaranteed to go through psgo all the time, don't we sometimes still exec ps?
While a new "space" added filed could cause an issue, it would be only a bad formatting on screen, not an actual bug.
Moving this function to psgo makes sense or having psgo expose all of these fields so we could get the list from it would make sense.
Podman is attempting to split the headers returned by the ps
command into a list of headers. Problem is that some headers
are multi-word, and headers are not guaranteed to be split via
a tab. This PR splits the headers bases on white space, and for
the select group of CAPS headers which are multi-word, combines
them back together.
Fixes: #17524
Does this PR introduce a user-facing change?