-
Notifications
You must be signed in to change notification settings - Fork 265
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
fix(container-detail): display the entire command instead of the first piece only #6604
base: main
Are you sure you want to change the base?
Conversation
…t piece Signed-off-by: GLEF1X <glebgar567@gmail.com>
Signed-off-by: GLEF1X <glebgar567@gmail.com>
@@ -42,7 +42,7 @@ export interface ContainerInfo { | |||
Names: string[]; | |||
Image: string; | |||
ImageID: string; | |||
Command?: string; | |||
Command?: readonly string[]; |
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.
From my understanding, this interface is inspired from dockerode definiton
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.
AFAIK, you're totally right. However, I changed it to readonly string[]
because listContainers
dynamically selects among various providers, leading to the use of workarounds such as podmanContainer.Command?.length > 0 ? podmanContainer.Command[0]: undefined
. In my opinion, ContainerInfo
should use an array to represent a command even though dockerode
returns a string because it enables us to not lose data(in this case other parts of the command).
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.
Yes I looked at the podman repository and they are indeed sending an array compared to dockerode definition.
I would be in favour to get something closer to what podman provide, the changes you are providing only touch the internal api, so it would not have much side effect, however the ContainerInfo
is also provided to the extension-api
.
export interface ContainerInfo { |
However for the extension-api
we have something different, with an interface called SimpleContainerInfo
which is some extending of dockerode.
export interface SimpleContainerInfo extends Dockerode.ContainerInfo { |
And I am wondering if we change/fix it at one place we would not have to propagate the changes also elsewhere... Waiting for @benoitf opinion on the matter
@@ -1031,7 +1031,7 @@ describe('listContainers', () => { | |||
expect(container.StartedAt).toBe('2023-08-10T13:37:44.000Z'); | |||
expect(container.pod).toBeUndefined(); | |||
expect(container.Id).toBe('31a4b282691420be2611817f203765402d8da7e13cd530f80a6ddd1bb4aa63b4'); | |||
expect(container.Command).toBe(undefined); | |||
expect(container.Command).toBe(null); |
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 am not sure to understand why null would be used here instead of undefined
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.
AFAIK, libpodApi
returns null
if the command is empty, whereas dockerode
returns undefined
Co-authored-by: axel7083 <42176370+axel7083@users.noreply.github.com> Signed-off-by: Hlib Haranin <71976818+GLEF1X@users.noreply.github.com>
Signed-off-by: GLEF1X <glebgar567@gmail.com>
Signed-off-by: GLEF1X glebgar567@gmail.com
What does this PR do?
This pull request modifies the UI to display all arguments passed to a container's run command rather than showing just the first piece for improved clarity on the processes being run within the container.
Screenshot / video of UI
Before
After
What issues does this PR fix or reference?
#6414 - Show all of Command in Summary
How to test this PR?