Skip to content

Comments

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

Merged
cpuguy83 merged 3 commits intomoby:masterfrom
thaJeztah:replace_term
Apr 22, 2020
Merged

Deprecate pkg/term and make it an alias for github.com/moby/term#40825
cpuguy83 merged 3 commits intomoby:masterfrom
thaJeztah:replace_term

Conversation

@thaJeztah
Copy link
Member

opening as draft, pending moby/term#4

vendor.conf Outdated
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.

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

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