Update vendoring of docker/docker #155

vdemeester · 2017-06-05T19:57:37Z

Updating the vendoring and moving some function over here (not used in moby/moby anymore).

Signed-off-by: Vincent Demeester [email protected]

aaronlehmann · 2017-06-05T20:01:25Z

vendor.conf

 github.com/docker/go-connections e15c02316c12de00874640cd76311849de2aeed5
 github.com/docker/go-events 18b43f1bc85d9cdd42c05a6cd2d444c7a200a894
 github.com/docker/go-units 9e638d38cf6977a37a8ea0078f3ee75a7cdb2dd1
-github.com/docker/libnetwork b13e0604016a4944025aaff521d9c125850b0d04
+github.com/docker/libnetwork 83e1e49475b88a9f1f8ba89a690a7d5de42e24b9


Seems like libnetwork is only used for a single IsLocalhost function in github.com/docker/libnetwork/resolvconf/dns.

Could we make the vendoring more specific, and reference that package directly? Or possibly copy that function into cli to avoid the dependency.

I would vote for copying the thing 👼

and updated 😉

codecov-io · 2017-06-05T20:08:13Z

Codecov Report

Merging #155 into master will decrease coverage by 0.11%.
The diff coverage is 17.1%.

@@            Coverage Diff             @@
##           master     #155      +/-   ##
==========================================
- Coverage   44.96%   44.84%   -0.12%     
==========================================
  Files         169      171       +2     
  Lines       11381    11464      +83     
==========================================
+ Hits         5117     5141      +24     
- Misses       5971     6028      +57     
- Partials      293      295       +2

dnephin · 2017-06-05T21:06:19Z

cli/command/service/opts.go

-			Name:   opts.name,
-			Labels: runconfigopts.ConvertKVStringsToMap(opts.labels.GetAll()),
+			Name:   options.name,
+			Labels: runconfigopts.ConvertKVStringsToMap(options.labels.GetAll()),


s/runconfigopts/opts/ ? (one more below this as well)

oh right :)

Signed-off-by: Vincent Demeester <[email protected]>

aaronlehmann · 2017-06-06T08:01:15Z

LGTM

When did we start enforcing code coverage targets for PRs? I thought we previously agreed not to do this: #109 (comment)

dnephin · 2017-06-06T14:49:17Z

I think it was enabled by that PR, I didn't realize that it would, but I don't think it's a problem.

Enforcement is up to reviewers. We don't have to wait for it to be green if we decide that the PR doesn't need test coverage, or that the PR is important enough to merge without coverage. A vendoring PR is a good example of when we might ignore it.

As far as I can tell enabling statuses is the only way to get the "patch coverage" percentage, which I think is the most valuable part of coverage. I'm glad we have it.

dnephin

LGTM

aaronlehmann · 2017-06-06T19:39:31Z

I think it was enabled by that PR, I didn't realize that it would, but I don't think it's a problem.

Enforcement is up to reviewers. We don't have to wait for it to be green if we decide that the PR doesn't need test coverage, or that the PR is important enough to merge without coverage. A vendoring PR is a good example of when we might ignore it.

As far as I can tell enabling statuses is the only way to get the "patch coverage" percentage, which I think is the most valuable part of coverage. I'm glad we have it.

I don't take any pleasure in arguing about this stuff, but I really don't like the idea of the "passing/failing checks" status becoming essentially random. IMHO, only reliable checks that we actually care about should count towards the status of the PR. If a PR is marked as failing its checks, there should be a good reason for it, not "oh, disregard that check because it's finicky and doesn't matter much". It would just be unfortunate and confusing if the status of the PR wasn't actually meaningful because of stuff like this. It means a lot of clicking through from the PR list to see if something's actually failing CI or not, and it's confusing to new contributors.

I'm also disappointed to see this marking PRs as failing their checks when I previously asked that we not do this, and was told there were no plans to.

vdemeester · 2017-06-06T19:42:56Z

@aaronlehmann @dnephin let's remove the "failure" checks for now and discuss this on a proposal, seems good ?

aaronlehmann · 2017-06-06T19:49:11Z

Wondering if it's possible to decrease the project and patch coverage targets to the point where the checks will always be marked successful, but then maintain informal coverage targets that we can apply more loosely.

This is the codecov.yml we use for swarmkit:

comment:
  layout: header, changes, diff, sunburst
coverage:
  status:
    patch: false
    project:
      default:
        enabled: yes
        target: 0
    changes: false
ignore:
  -**/testutils

Perhaps something similar (without patch having target: 0 instead of being disabled) would work?

dnephin · 2017-06-06T19:50:27Z

let's remove the "failure" checks for now

Is there any other way of seeing patch coverage?

vdemeester · 2017-06-06T19:51:24Z

@aaronlehmann works for me. Another way is to augment the threshold (threshold: "10%" for ex). That way it will fail in the PR decrease the coverage by 10% (which would be quite bad)

aaronlehmann · 2017-06-07T11:25:51Z

LGTM

…-19.03-a63faebcf13973933a3ae646e5a9e5befaad42fd [19.03] sync to upstream 19.03 a63faeb

This makes a quick pass through our tests; Discard output/err ---------------------------------------------- Many tests were testing for error-conditions, but didn't discard output. This produced a lot of noise when running the tests, and made it hard to discover if there were actual failures, or if the output was expected. For example: === RUN TestConfigCreateErrors Error: "create" requires exactly 2 arguments. See 'create --help'. Usage: create [OPTIONS] CONFIG file|- [flags] Create a config from a file or STDIN Error: "create" requires exactly 2 arguments. See 'create --help'. Usage: create [OPTIONS] CONFIG file|- [flags] Create a config from a file or STDIN Error: error creating config --- PASS: TestConfigCreateErrors (0.00s) And after discarding output: === RUN TestConfigCreateErrors --- PASS: TestConfigCreateErrors (0.00s) Use sub-tests where possible ---------------------------------------------- Some tests were already set-up to use test-tables, and even had a usable name (or in some cases "error" to check for). Change them to actual sub- tests. Same test as above, but now with sub-tests and output discarded: === RUN TestConfigCreateErrors === RUN TestConfigCreateErrors/requires_exactly_2_arguments === RUN TestConfigCreateErrors/requires_exactly_2_arguments#01 === RUN TestConfigCreateErrors/error_creating_config --- PASS: TestConfigCreateErrors (0.00s) --- PASS: TestConfigCreateErrors/requires_exactly_2_arguments (0.00s) --- PASS: TestConfigCreateErrors/requires_exactly_2_arguments#01 (0.00s) --- PASS: TestConfigCreateErrors/error_creating_config (0.00s) PASS It's not perfect in all cases (in the above, there's duplicate "expected" errors, but Go conveniently adds "#1" for the duplicate). There's probably also various tests I missed that could still use the same changes applied; we can improve these in follow-ups. Set cmd.Args to prevent test-failures ---------------------------------------------- When running tests from my IDE, it compiles the tests before running, then executes the compiled binary to run the tests. Cobra doesn't like that, because in that situation `os.Args` is taken as argument for the command that's executed. The command that's tested now sees the test- flags as arguments (`-test.v -test.run ..`), which causes various tests to fail ("Command XYZ does not accept arguments"). # compile the tests: go test -c -o foo.test # execute the test: ./foo.test -test.v -test.run TestFoo === RUN TestFoo Error: "foo" accepts no arguments. The Cobra maintainers ran into the same situation, and for their own use have added a special case to ignore `os.Args` in these cases; https://github.com/spf13/cobra/blob/v1.8.1/command.go#L1078-L1083 args := c.args // Workaround FAIL with "go test -v" or "cobra.test -test.v", see docker#155 if c.args == nil && filepath.Base(os.Args[0]) != "cobra.test" { args = os.Args[1:] } Unfortunately, that exception is too specific (only checks for `cobra.test`), so doesn't automatically fix the issue for other test-binaries. They did provide a `cmd.SetArgs()` utility for this purpose https://github.com/spf13/cobra/blob/v1.8.1/command.go#L276-L280 // SetArgs sets arguments for the command. It is set to os.Args[1:] by default, if desired, can be overridden // particularly useful when testing. func (c *Command) SetArgs(a []string) { c.args = a } And the fix is to explicitly set the command's args to an empty slice to prevent Cobra from falling back to using `os.Args[1:]` as arguments. cmd := newSomeThingCommand() cmd.SetArgs([]string{}) Some tests already take this issue into account, and I updated some tests for this, but there's likely many other ones that can use the same treatment. Perhaps the Cobra maintainers would accept a contribution to make their condition less specific and to look for binaries ending with a `.test` suffix (which is what compiled binaries usually are named as). Signed-off-by: Sebastiaan van Stijn <[email protected]>

This makes a quick pass through our tests; Discard output/err ---------------------------------------------- Many tests were testing for error-conditions, but didn't discard output. This produced a lot of noise when running the tests, and made it hard to discover if there were actual failures, or if the output was expected. For example: === RUN TestConfigCreateErrors Error: "create" requires exactly 2 arguments. See 'create --help'. Usage: create [OPTIONS] CONFIG file|- [flags] Create a config from a file or STDIN Error: "create" requires exactly 2 arguments. See 'create --help'. Usage: create [OPTIONS] CONFIG file|- [flags] Create a config from a file or STDIN Error: error creating config --- PASS: TestConfigCreateErrors (0.00s) And after discarding output: === RUN TestConfigCreateErrors --- PASS: TestConfigCreateErrors (0.00s) Use sub-tests where possible ---------------------------------------------- Some tests were already set-up to use test-tables, and even had a usable name (or in some cases "error" to check for). Change them to actual sub- tests. Same test as above, but now with sub-tests and output discarded: === RUN TestConfigCreateErrors === RUN TestConfigCreateErrors/requires_exactly_2_arguments === RUN TestConfigCreateErrors/requires_exactly_2_arguments#01 === RUN TestConfigCreateErrors/error_creating_config --- PASS: TestConfigCreateErrors (0.00s) --- PASS: TestConfigCreateErrors/requires_exactly_2_arguments (0.00s) --- PASS: TestConfigCreateErrors/requires_exactly_2_arguments#01 (0.00s) --- PASS: TestConfigCreateErrors/error_creating_config (0.00s) PASS It's not perfect in all cases (in the above, there's duplicate "expected" errors, but Go conveniently adds "#1" for the duplicate). There's probably also various tests I missed that could still use the same changes applied; we can improve these in follow-ups. Set cmd.Args to prevent test-failures ---------------------------------------------- When running tests from my IDE, it compiles the tests before running, then executes the compiled binary to run the tests. Cobra doesn't like that, because in that situation `os.Args` is taken as argument for the command that's executed. The command that's tested now sees the test- flags as arguments (`-test.v -test.run ..`), which causes various tests to fail ("Command XYZ does not accept arguments"). # compile the tests: go test -c -o foo.test # execute the test: ./foo.test -test.v -test.run TestFoo === RUN TestFoo Error: "foo" accepts no arguments. The Cobra maintainers ran into the same situation, and for their own use have added a special case to ignore `os.Args` in these cases; https://github.com/spf13/cobra/blob/v1.8.1/command.go#L1078-L1083 args := c.args // Workaround FAIL with "go test -v" or "cobra.test -test.v", see docker#155 if c.args == nil && filepath.Base(os.Args[0]) != "cobra.test" { args = os.Args[1:] } Unfortunately, that exception is too specific (only checks for `cobra.test`), so doesn't automatically fix the issue for other test-binaries. They did provide a `cmd.SetArgs()` utility for this purpose https://github.com/spf13/cobra/blob/v1.8.1/command.go#L276-L280 // SetArgs sets arguments for the command. It is set to os.Args[1:] by default, if desired, can be overridden // particularly useful when testing. func (c *Command) SetArgs(a []string) { c.args = a } And the fix is to explicitly set the command's args to an empty slice to prevent Cobra from falling back to using `os.Args[1:]` as arguments. cmd := newSomeThingCommand() cmd.SetArgs([]string{}) Some tests already take this issue into account, and I updated some tests for this, but there's likely many other ones that can use the same treatment. Perhaps the Cobra maintainers would accept a contribution to make their condition less specific and to look for binaries ending with a `.test` suffix (which is what compiled binaries usually are named as). Signed-off-by: Sebastiaan van Stijn <[email protected]> (cherry picked from commit ab23024) Signed-off-by: Sebastiaan van Stijn <[email protected]>

vdemeester requested review from dnephin, cpuguy83, aaronlehmann and mlaventure June 5, 2017 19:57

GordonTheTurtle added the status/0-triage label Jun 5, 2017

aaronlehmann reviewed Jun 5, 2017

View reviewed changes

vdemeester force-pushed the fix-and-update-vendor branch from d38fbe6 to dc7dd9f Compare June 5, 2017 20:57

vdemeester added status/2-code-review and removed status/0-triage labels Jun 5, 2017

dnephin reviewed Jun 5, 2017

View reviewed changes

Update vendoring of docker/docker

44ac808

Signed-off-by: Vincent Demeester <[email protected]>

vdemeester force-pushed the fix-and-update-vendor branch from dc7dd9f to 44ac808 Compare June 5, 2017 22:23

dnephin approved these changes Jun 6, 2017

View reviewed changes

aaronlehmann merged commit 583ed2e into docker:master Jun 7, 2017

GordonTheTurtle added this to the 17.06.0 milestone Jun 7, 2017

vdemeester deleted the fix-and-update-vendor branch June 7, 2017 11:27

vdemeester mentioned this pull request Jun 7, 2017

Update codecov project threshold to 15% #161

Merged

thaJeztah modified the milestones: 17.07.0, 17.06.0 Jun 8, 2017

This was referenced Jun 14, 2017

17.06.0 RC4 tracker docker/for-linux#14

Closed

17.06.0 RC5 tracker docker/for-linux#37

Closed

thaJeztah mentioned this pull request Jul 30, 2019

Releases / Tags for this repository and associated repositories moby/moby#39632

Closed

trapier pushed a commit to trapier/cli that referenced this pull request Sep 30, 2019

Merge pull request docker#155 from GordonTheTurtle/sync-with-upstream…

51be736

…-19.03-a63faebcf13973933a3ae646e5a9e5befaad42fd [19.03] sync to upstream 19.03 a63faeb

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update vendoring of docker/docker #155

Update vendoring of docker/docker #155

vdemeester commented Jun 5, 2017

aaronlehmann Jun 5, 2017

vdemeester Jun 5, 2017

vdemeester Jun 5, 2017

codecov-io commented Jun 5, 2017 •

edited

Loading

dnephin Jun 5, 2017

vdemeester Jun 5, 2017

aaronlehmann commented Jun 6, 2017

dnephin commented Jun 6, 2017

dnephin left a comment

aaronlehmann commented Jun 6, 2017

vdemeester commented Jun 6, 2017

aaronlehmann commented Jun 6, 2017

dnephin commented Jun 6, 2017

vdemeester commented Jun 6, 2017

aaronlehmann commented Jun 7, 2017

Update vendoring of docker/docker #155

Update vendoring of docker/docker #155

Conversation

vdemeester commented Jun 5, 2017

aaronlehmann Jun 5, 2017

Choose a reason for hiding this comment

vdemeester Jun 5, 2017

Choose a reason for hiding this comment

vdemeester Jun 5, 2017

Choose a reason for hiding this comment

codecov-io commented Jun 5, 2017 • edited Loading

Codecov Report

dnephin Jun 5, 2017

Choose a reason for hiding this comment

vdemeester Jun 5, 2017

Choose a reason for hiding this comment

aaronlehmann commented Jun 6, 2017

dnephin commented Jun 6, 2017

dnephin left a comment

Choose a reason for hiding this comment

aaronlehmann commented Jun 6, 2017

vdemeester commented Jun 6, 2017

aaronlehmann commented Jun 6, 2017

dnephin commented Jun 6, 2017

vdemeester commented Jun 6, 2017

aaronlehmann commented Jun 7, 2017

codecov-io commented Jun 5, 2017 •

edited

Loading