-
Notifications
You must be signed in to change notification settings - Fork 203
chore: convert console output to creack/pty #1850
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
base: main
Are you sure you want to change the base?
chore: convert console output to creack/pty #1850
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1850 +/- ##
==========================================
- Coverage 85.28% 84.82% -0.46%
==========================================
Files 143 143
Lines 6742 6769 +27
==========================================
- Hits 5750 5742 -8
- Misses 705 731 +26
- Partials 287 296 +9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
9e0f443
to
b978cdb
Compare
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.
Pull Request Overview
This PR migrates from the inactive containerd/console
library to the more actively maintained creack/pty
library for pseudo-terminal functionality, addressing potential Mac compatibility issues.
- Replaces
containerd/console
withcreack/pty
across the codebase - Updates console interface and implementation to use the new library
- Modifies test utilities and test cases to work with the new PTY library
Reviewed Changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated no comments.
File | Description |
---|---|
go.mod | Updates dependency from containerd/console to creack/pty |
internal/testutils/console.go | Refactors PTY creation and matching utilities to use creack/pty |
cmd/oras/internal/display/status/console/console.go | Implements new console interface using creack/pty and golang.org/x/term |
cmd/oras/internal/display/status/console/console_test.go | Updates all test functions to use new PTY interface and terminology |
Comments suppressed due to low confidence (2)
go.mod:1
- Go version 1.25.0 does not exist. The latest stable Go version as of January 2025 was 1.23.x. Please use a valid Go version.
module oras.land/oras
cmd/oras/internal/display/status/console/console.go:1
- The GetHeightWidth method is missing its implementation. This will cause a compilation error as the method signature exists but has no body.
/*
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Should the PR title start with "refactor"? |
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 with nits
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 with minor suggestions.
It would be a good start on extending test coverage in Darwin. Thanks for your contribution. @TerryHowe |
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.
Taking approvement back. I tested on Windows and the tty fails. Confirming offline.
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. Sry I used a bad build to test on Windows. The actual build in this PR works fine.
The width on Window not the actual size though, the width is always 80. @Wwwsylvia @shizhMSFT What do you think? ![]() |
Good point, fixed. This happenwd to be a case where the code was nicely encapsulated, so went easier than expected! |
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.
The width on Window not the actual size though, the width is always 80
I've reproduced that. Good catch. It's a regression. We need to find out a way to fix it.
@TerryHowe After investigating |
Signed-off-by: Terry Howe <[email protected]>
7d32e57
to
27285ce
Compare
Signed-off-by: Terry Howe <[email protected]>
27285ce
to
5fcc0a3
Compare
What this PR does / why we need it:

The https://github.com/containerd/console we are using is pretty much a dead project. There has been a ticket about mac failures for over a year and no action over there other than bug fixes. The project is small time:
The creack/pty project is a lot more active:

Closes: #1449