Skip to content

Switch from x/net/context -> context#2305

Merged
dmcgowan merged 3 commits intocontainerd:masterfrom
kolyshkin:context
Apr 25, 2018
Merged

Switch from x/net/context -> context#2305
dmcgowan merged 3 commits intocontainerd:masterfrom
kolyshkin:context

Conversation

@kolyshkin
Copy link
Copy Markdown
Contributor

Since Go 1.7, "context" is a standard package, superceding the "x/net/context". Since Go 1.9, the latter only provides type aliases from the former. Therefore, it makes sense to switch to the standard package, and the change is not disruptive in any sense.

The first commit deals with a few cases where both packages happened to be imported by the same source file. A choice between "context" and "gocontext" was made for each file in order to minimize the patch.

The second commit is generated by a shell script, and deals with the rest of the code.

Similar changes to moby/moby: moby/moby#36904.

@crosbymichael
Copy link
Copy Markdown
Member

We tried this before but because GRPC still uses the old context, the service implementations will not compile because they don't fulfill the interface.

@mlaventure
Copy link
Copy Markdown
Contributor

Didn't they update go fix to make use of alias when updating these?

@cpuguy83
Copy link
Copy Markdown
Member

For go1.9+ x/net/context it's just an alias to to the stdlib context.

@kolyshkin
Copy link
Copy Markdown
Contributor Author

Sorry I missed it, needed to re-vendor x/net, commit added.

@kolyshkin
Copy link
Copy Markdown
Contributor Author

rebased; changed the order of commits to not break git bisect

@AkihiroSuda
Copy link
Copy Markdown
Member

Do we need to cherry-pick this to v1.1.1 for using containerd 1.1 in Moby?

This version includes "x/net/context" which is fully compatible with
the standard Go "context" package, so the two can be mixed together.

Signed-off-by: Kir Kolyshkin <[email protected]>
Since Go 1.7, "context" is a standard package, superceding the
"x/net/context". Since Go 1.9, the latter only provides type aliases
from the former. Therefore, it makes sense to switch to the standard
package, and the change is not disruptive in any sense.

This commit deals with a few cases where both packages happened to be
imported by the same source file. A choice between "context" and
"gocontext" was made for each file in order to minimize the patch.

Signed-off-by: Kir Kolyshkin <[email protected]>
Since Go 1.7, context is a standard package, superceding the
"x/net/context". Since Go 1.9, the latter only provides a few type
aliases from the former. Therefore, it makes sense to switch to the
standard package.

This commit was generated by the following script (with a couple of
minor fixups to remove extra changes done by goimports):

	#!/bin/bash

	if [ $# -ge 1 ]; then
		FILES=$*
	else
		FILES=$(git ls-files \*.go | grep -vF ".pb.go" | grep -v
	^vendor/)
	fi

	for f in $FILES; do
		printf .
		sed -i -e 's|"golang.org/x/net/context"$|"context"|' $f
		goimports -w $f
		awk '	/^$/ {e=1; next;}
			/[[:space:]]"context"$/ {e=0;}
			{if (e) {print ""; e=0}; print;}' < $f > $f.new && \
				mv $f.new $f
		goimports -w $f
	done
	echo

Signed-off-by: Kir Kolyshkin <[email protected]>
@codecov-io
Copy link
Copy Markdown

Codecov Report

Merging #2305 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2305   +/-   ##
=======================================
  Coverage   45.54%   45.54%           
=======================================
  Files          83       83           
  Lines        9185     9185           
=======================================
  Hits         4183     4183           
  Misses       4326     4326           
  Partials      676      676
Flag Coverage Δ
#linux 50.07% <ø> (ø) ⬆️
#windows 41.24% <ø> (ø) ⬆️
Impacted Files Coverage Δ
server/server.go 2.27% <ø> (ø) ⬆️
namespaces/grpc.go 62.5% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 209a7fc...bbe14f0. Read the comment docs.

@crosbymichael
Copy link
Copy Markdown
Member

LGTM

@dmcgowan
Copy link
Copy Markdown
Member

LGTM

Do we need to cherry-pick this to v1.1.1 for using containerd 1.1 in Moby?

What would be the concern here? My understanding is there is no functional difference and it is already compiling with Moby no problem.

@dmcgowan dmcgowan added this to the 1.2 milestone Apr 25, 2018
@dmcgowan dmcgowan merged commit 1a5e0df into containerd:master Apr 25, 2018
@AkihiroSuda
Copy link
Copy Markdown
Member

it is already compiling with Moby no problem.

👍

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.

7 participants