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.

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

feat: use tty for containers started with TTY/interactive mode #3900

Merged
merged 1 commit into from Sep 19, 2023

Conversation

benoitf
Copy link
Collaborator

@benoitf benoitf commented Sep 14, 2023

What does this PR do?

Allow to use the interactive terminal of a container (and not a new bash terminal)

note: if you start with interactive mode, if you start a container,
you are redirected to the tty tab of the container

if you click on the details of a container, you're redirected
to the tty tab if the container has been launched using tty/interactive
else you see the logs

In the details of a container, tty tab is only available if
container is started with tty/interactive mode else it's hidden.

Screenshot/screencast of this PR

What issues does this PR fix or reference?

Fixes #970

How to test this PR?

pull an image like fedora then run a new container from this image (in image list, click on the ▶️ icon)
then you should have a terminal

try also with node image. Here you've the interactive nodejs prompt while if you use the terminal tab you have sh/bash prompt.

unit tests added

Copy link
Contributor

@lstocchi lstocchi left a comment

Choose a reason for hiding this comment

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

What i don't like much as a first comment is the fact that i see a black page for some seconds before the details page is rendered. Maybe we can add a loading so the user knows something is happening.
black_page

note: if you start with interactive mode, if you start a container,
you are redirected to the tty tab of the container

if you click on the details of a container, you're redirected
to the tty tab if the container has been launched using tty/interactive
else you see the logs

In the details of a container, tty tab is only available if
container is started with tty/interactive mode else it's hidden.

fixes containers#970
Signed-off-by: Florent Benoit <fbenoit@redhat.com>
Copy link
Contributor

@lstocchi lstocchi left a comment

Choose a reason for hiding this comment

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

Works fine for me !! 🚀

@benoitf
Copy link
Collaborator Author

benoitf commented Sep 18, 2023

@lstocchi I don't know if we need a follow-up PR or tackle the 'loading' screen before merging.

I think it's around the current design of the page where we don't display anything if we don't have all the data.

I agree that the experience is not nice/smooth. I had the same feeling.

But probably we should see almost all the layout, etc and the loading should occur only on the "terminal" part but here due to the design we don't even see the title of the container

@lstocchi
Copy link
Contributor

@lstocchi I don't know if we need a follow-up PR or tackle the 'loading' screen before merging.

I think it's around the current design of the page where we don't display anything if we don't have all the data.

I agree that the experience is not nice/smooth. I had the same feeling.

But probably we should see almost all the layout, etc and the loading should occur only on the "terminal" part but here due to the design we don't even see the title of the container

Ok to follow-up for me. Not a blocker imo. Maybe we should add an issue to keep track of it or a general one to investigate which pages have a longer loading and need a ui element to keep the user informed somehow.

Copy link
Contributor

@jeffmaury jeffmaury left a comment

Choose a reason for hiding this comment

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

LGTM

@benoitf benoitf merged commit bff540c into containers:main Sep 19, 2023
8 checks passed
@podman-desktop-bot podman-desktop-bot added this to the 1.5.0 milestone Sep 19, 2023
@benoitf
Copy link
Collaborator Author

benoitf commented Sep 19, 2023

created #3975

@benoitf benoitf deleted the DESKTOP-970-e branch September 20, 2023 07:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Open terminal screen if tty is selected in create container
4 participants