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

Deprecate pkg/term and make it an alias for github.com/moby/term #40825

Merged
merged 3 commits into from
Apr 22, 2020

Conversation

thaJeztah
Copy link
Member

opening as draft, pending moby/term#4

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
Copy link
Member Author

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

Copy link
Member

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

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

LGTM

@thaJeztah
Copy link
Member Author

Hmm.. ppc64le nodes are failing again with networking issues;


[2020-04-16T18:52:43.065Z] fatal: unable to access 'https://github.com/docker/buildx.git/': Could not resolve host: github.com
[2020-04-16T18:52:43.065Z] The command '/bin/sh -c git clone "${BUILDX_REPO}" /buildx' returned a non-zero code: 128
[2020-04-16T18:52:43.065Z] Makefile:279: recipe for target 'bundles/buildx' failed
[2020-04-16T18:52:43.065Z] make: *** [bundles/buildx] Error 128

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.

@thaJeztah
Copy link
Member Author

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/

Screenshot 2020-04-17 at 12 35 25

@kolyshkin
Copy link
Contributor

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.

@kolyshkin
Copy link
Contributor

Also it looks like the package-wide deprecation notice is missing.

@thaJeztah
Copy link
Member Author

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.

I swapped the last two commits

Also it looks like the package-wide deprecation notice is missing.

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"

@kolyshkin
Copy link
Contributor

I swapped the last two commits

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).

Does golang support deprecating the package as a whole, or only individual functions?

Yes ("or even a whole package"):
https://github.com/golang/go/wiki/Deprecated

Wondering if it adds anything if all functions are already marked "deprecated"

I think it does:

  1. Package-level documentation would start with the notice this is deprecated.
  2. A mere import of the package would be considered a linter issue.
  3. Maybe it's easier to deprecate the whole package at once.

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>
@thaJeztah
Copy link
Member Author

@kolyshkin updated; added deprecation message for the package

Copy link
Contributor

@tao12345666333 tao12345666333 left a 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 (
Copy link
Contributor

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

Copy link
Contributor

@kolyshkin kolyshkin left a 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)

@cpuguy83 cpuguy83 merged commit f6a5ccf into moby:master Apr 22, 2020
@thaJeztah thaJeztah deleted the replace_term branch April 22, 2020 06:56
@thaJeztah thaJeztah added this to the 20.03.0 milestone May 6, 2020
greut added a commit to greut/nomad that referenced this pull request Jun 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants