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

fix nerdctl exec init console size 0 0 problem #2182

Merged
merged 1 commit into from
Apr 17, 2023
Merged

fix nerdctl exec init console size 0 0 problem #2182

merged 1 commit into from
Apr 17, 2023

Conversation

ChangJH-Mark
Copy link
Contributor

currently nerdctl exec -it initial tty size is 0 0, switch it to current console size

@@ -181,3 +184,11 @@ func generateExecProcessSpec(ctx context.Context, client *containerd.Client, con

return pspec, nil
}

func setInitialTTYSize(pspec *specs.Process) {
Copy link
Member

Choose a reason for hiding this comment

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

please see

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's a function works more like specs.Process' decorator, so it's not so proper to go to consoleutil

func setInitialTTYSize(pspec *specs.Process) {
size, err := console.Current().Size()
if err != nil {
size.Height, size.Width = 0, 0
Copy link
Member

Choose a reason for hiding this comment

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

why override with 0, 0 values ?
may be

	size, err := console.Current().Size()
	if err == nil {
		pspec.ConsoleSize = &specs.Box{Height: uint(size.Height), Width: uint(size.Width)}
       }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's right, I've change codes now

@fahedouch
Copy link
Member

fahedouch commented Apr 15, 2023

Hello,

you previously opened two sililar PR ( 1 & 2 ). Are you facing some problems, can we help ?

@fahedouch fahedouch added this to the v1.4.0 (tentative) milestone Apr 15, 2023
@fahedouch
Copy link
Member

would you please squash your commits, you can ping me if you need some help. Thanks

Signed-off-by: ChangJH-Mark <1431293514@qq.com>
@ChangJH-Mark
Copy link
Contributor Author

Hello,

you previously opened two sililar PR ( 1 & 2 ). Are you facing some problems, can we help ?

well, I delete PR remote branch mistakenly, because of trying to squash commits and can't push unforcely. Anyway it's a mistake, I realize the how to update an remote PR branch now, thanks for asking.

@ChangJH-Mark ChangJH-Mark reopened this Apr 16, 2023
Copy link
Member

@fahedouch fahedouch left a comment

Choose a reason for hiding this comment

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

Thanks

@fahedouch fahedouch merged commit 1e25ba7 into containerd:main Apr 17, 2023
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.

2 participants