-
Notifications
You must be signed in to change notification settings - Fork 18.7k
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
Deprecate pkg/term and make it an alias for github.com/moby/term #40825
Conversation
vendor.conf
Outdated
@@ -6,7 +6,7 @@ github.com/golang/gddo 72a348e765d293ed6d1ded7b6995 | |||
github.com/google/uuid 0cd6bf5da1e1c83f8b45653022c74f71af0538a4 # v1.1.1 | |||
github.com/gorilla/mux 00bdffe0f3c77e27d2cf6f5c70232a2d3e4d9c15 # v1.7.3 | |||
github.com/Microsoft/opengcs a10967154e143a36014584a6f664344e3bb0aa64 | |||
github.com/moby/term c95b5c60a749a1e7cfdee1b5961962e423fbe8ec | |||
github.com/moby/term d92e24e4bb4d6595f89f0fe52a76292f8494d787 https://github.com/thaJeztah/term |
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.
Temporarily using my fork until the linked PR is merged
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
Hmm.. ppc64le nodes are failing again with networking issues;
This was on this node: https://ci-next.docker.com/public/computer/ppc64le-ubuntu-19/systemInfo - looks like all previous failures were on that node as well; let me disable it to see if it is just that one. |
Ah; I see some workers ran out of swap space; are we missing some cleanup step in CI? https://ci-next.docker.com/public/computer/ |
I would put "pkg/term: vendor moby/term and make pkg/term an alias" commit first and test with it (to make sure the deprecated stuff works) and only after it add the "remove uses of deprecated pkg/term" commit. Since Github web UI does not show the patches in the order they are applied, please ignore this if commits are already in the order described above. |
Also it looks like the package-wide deprecation notice is missing. |
I swapped the last two commits
Does golang support deprecating the package as a whole, or only individual functions? 🤔 Wondering if it adds anything if all functions are already marked "deprecated" |
Thanks! It also need to test it with the commit that starts using the deprecated package (it should at least build, this is a sure way to know the package is working and nothing is amiss).
Yes ("or even a whole package"):
I think it does:
|
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@kolyshkin updated; added deprecation message for the package |
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. 👍
// Deprecated: use github.com/moby/term.Winsize | ||
type Winsize = term.Winsize | ||
|
||
var ( |
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.
If I remember correctly, for a group like this you can do
// Deprecated: use github.com/moby/term instead
var (
...
)
and then you don't have to document/deprecate each individual one.
Not a big deal anyway
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 (I also checked it builds before the last commit)
See moby/moby#40825 Signed-off-by: Yoan Blanc <yoan@dosimple.ch>
opening as draft, pending moby/term#4