Migrate test-integration-cli experimental ipvlan test to integration by vdemeester · Pull Request #36722 · moby/moby

vdemeester · 2018-03-28T15:55:29Z

~~Build on top of #36697, only the second commit (007b7f7) should be reviewed 👼~~

All Ipvlan related test on DockerSuite and DockerNetworkSuite
are migrated to ipvlan_test.go.

The end goal being to remove the experimental builds.

It also refactors macvlan tests a bit (quicker) and it put macvlan and ipvlan tests on there own folder in order to be isolated and have quick "suite).

cc @fcrisciani @selansen

🦁

Signed-off-by: Vincent Demeester vincent@sbr.pm

codecov · 2018-03-29T10:52:37Z

Codecov Report

❗ No coverage uploaded for pull request base (master@59d5608). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master   #36722   +/-   ##
=========================================
  Coverage          ?   34.92%           
=========================================
  Files             ?      614           
  Lines             ?    45644           
  Branches          ?        0           
=========================================
  Hits              ?    15943           
  Misses            ?    27607           
  Partials          ?     2094

vdemeester · 2018-03-29T13:13:54Z

Updated to do a bit more than just ipvlan migration. It also refactors macvlan tests a bit (quicker) and it put macvlan and ipvlan tests on there own folder in order to be isolated and have quick "suite).

fcrisciani · 2018-04-06T19:50:01Z

@ctelfer can you take a look too?

fcrisciani

high level I guess looks good

ctelfer

Sorry it has taken a while to get through the code. LGTM overall. Just one comment about a cleanup function.

ctelfer · 2018-04-06T21:20:48Z

integration/network/helpers.go

This says delete a network interface, but these iptables commands wipe every rule in the nat & filter tables of the entire system (or at least namespace). Is this intended? Might consider updating the name if so.

@ctelfer oh interesting… I took what was previously here so the behavior doesn't change from previous tests. cc @mavenugo to get more insight 👼

As discussed on slack: these tests set up a new daemon, so this shouldn't be a problem, but perhaps we can add a TODO to these (as a follow-up)

The problem is these daemons aren't in their own network namespace and it's flushing everything in the main namespace.

thaJeztah

LGTM, left some nits/questions but it's ok to address those separately

thaJeztah · 2018-04-10T16:37:30Z

integration/network/ipvlan_test.go

Commented on another PR; we should go over the tests and be consistent (either "!= linux" or "== windows").

No need to fix for this PR, as it's a general thing to look at

thaJeztah · 2018-04-10T16:38:09Z

integration/network/macvlan_test.go

Guess this can be removed

thaJeztah · 2018-04-10T16:41:40Z

integration/network/macvlan_test.go

Wondering if we should start moving comments like this as custom message (not for this PR)

thaJeztah · 2018-04-10T16:43:09Z

integration/network/helpers.go

As discussed on slack: these tests set up a new daemon, so this shouldn't be a problem, but perhaps we can add a TODO to these (as a follow-up)

thaJeztah · 2018-04-10T16:45:51Z

integration/network/helpers.go

Should probably be moved somewhere else, as it's not network-specific (may even be another helper like this already)

cpuguy83

LGTM

cpuguy83 · 2018-04-10T17:09:05Z

And a conflict now.

vdemeester · 2018-04-10T17:12:52Z

Gonna fix the conflicts and take @thaJeztah comments into account tomorrow morning (Paris time 😉)

All `Ipvlan` related test on `DockerSuite` and `DockerNetworkSuite` are migrated to `ipvlan_test.go`. The end goal being to remove the `experimental` builds. Signed-off-by: Vincent Demeester <vincent@sbr.pm>

Signed-off-by: Vincent Demeester <vincent@sbr.pm>

… making each folder/suites quicker to run Signed-off-by: Vincent Demeester <vincent@sbr.pm>

vdemeester · 2018-04-11T09:15:46Z

Rebased 👼

vdemeester requested review from cpuguy83, dnephin, thaJeztah and yongtang March 28, 2018 15:55

GordonTheTurtle added the status/0-triage label Mar 28, 2018

vdemeester force-pushed the network-ipvlan-test-migration branch from 007b7f7 to f23cde0 Compare March 28, 2018 15:57

vdemeester added status/2-code-review and removed status/0-triage labels Mar 28, 2018

yongtang added rebuild/janky labels Mar 28, 2018

GordonTheTurtle removed rebuild/experimental labels Mar 28, 2018

vdemeester added rebuild/janky labels Mar 29, 2018

GordonTheTurtle removed rebuild/experimental labels Mar 29, 2018

vdemeester force-pushed the network-ipvlan-test-migration branch 4 times, most recently from 398e75b to 5a4dacc Compare March 29, 2018 13:11

vdemeester force-pushed the network-ipvlan-test-migration branch from 5a4dacc to f4a850b Compare March 29, 2018 13:31

vdemeester added the rebuild/powerpc label Mar 30, 2018

GordonTheTurtle removed the rebuild/powerpc label Mar 30, 2018

vdemeester mentioned this pull request Apr 5, 2018

CI enhancement and spring cleaning #36416

Open

17 tasks

thaJeztah added area/networking Networking area/testing labels Apr 6, 2018

fcrisciani approved these changes Apr 7, 2018

View reviewed changes

ctelfer reviewed Apr 9, 2018

View reviewed changes

thaJeztah approved these changes Apr 10, 2018

View reviewed changes

thaJeztah mentioned this pull request Apr 10, 2018

Move integration-cli daemon package to internal/test… #36824

Merged

cpuguy83 approved these changes Apr 10, 2018

View reviewed changes

vdemeester added 3 commits April 11, 2018 11:14

Migrate test-integration-cli experimental ipvlan test to integration

24f9347

All `Ipvlan` related test on `DockerSuite` and `DockerNetworkSuite` are migrated to `ipvlan_test.go`. The end goal being to remove the `experimental` builds. Signed-off-by: Vincent Demeester <vincent@sbr.pm>

Refactor macvlan tests a bit

0ab6116

Signed-off-by: Vincent Demeester <vincent@sbr.pm>

Move ipvlan and macvlan tests on their own folder…

a3323d2

… making each folder/suites quicker to run Signed-off-by: Vincent Demeester <vincent@sbr.pm>

vdemeester force-pushed the network-ipvlan-test-migration branch from f4a850b to a3323d2 Compare April 11, 2018 09:15

vdemeester added the rebuild/powerpc label Apr 11, 2018

GordonTheTurtle removed the rebuild/powerpc label Apr 11, 2018

vdemeester added the rebuild/powerpc label Apr 11, 2018

GordonTheTurtle removed the rebuild/powerpc label Apr 11, 2018

thaJeztah added status/4-merge and removed status/2-code-review labels Apr 11, 2018

yongtang added the rebuild/powerpc label Apr 11, 2018

GordonTheTurtle removed the rebuild/powerpc label Apr 11, 2018

thaJeztah merged commit 6f4bf56 into moby:master Apr 11, 2018

vdemeester deleted the network-ipvlan-test-migration branch April 12, 2018 07:43

thaJeztah mentioned this pull request Apr 18, 2018

[18.03] Updates to integration tests, migrate to "gotestyourself" docker-archive/docker-ce#513

Merged

Conversation

vdemeester commented Mar 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Mar 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

vdemeester commented Mar 29, 2018

Uh oh!

fcrisciani commented Apr 6, 2018

Uh oh!

fcrisciani left a comment

Choose a reason for hiding this comment

Uh oh!

ctelfer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cpuguy83 left a comment

Choose a reason for hiding this comment

Uh oh!

cpuguy83 commented Apr 10, 2018

Uh oh!

vdemeester commented Apr 10, 2018

Uh oh!

vdemeester commented Apr 11, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

vdemeester commented Mar 28, 2018 •

edited

Loading

codecov bot commented Mar 29, 2018 •

edited

Loading