Skip to content

Conversation

@cpuguy83
Copy link
Member

@cpuguy83 cpuguy83 commented Mar 18, 2021

This seems to be testing a strange case, specifically that one can set
the --net and --network in the same command with the same network.

Indeed this used to work with older CLIs but newer ones error out when
validating the request before sending it to the daemon.

Opening this for discussion because:

  1. This doesn't seem to be testing anything at all related to the rest
    of the test
  2. Not really providing any value here.
  3. Is testing that a technically invalid option is successful (whether
    the option should be valid as it relates to the CLI accepting it is
    debatable).
  4. Such a case seems fringe and even a bug in whatever is calling the
    CLI with such options.

The change that caused this is from docker/cli#1767

@cpuguy83 cpuguy83 requested a review from tiborvass March 18, 2021 17:14
This seems to be testing a strange case, specifically that one can set
the `--net` and `--network` in the same command with the same network.

Indeed this used to work with older CLIs but newer ones error out when
validating the request before sending it to the daemon.

Opening this for discussion because:

1. This doesn't seem to be testing anything at all related to the rest
   of the test
2. Not really providing any value here.
3. Is testing that a technically invalid option is successful (whether
   the option should be valid as it relates to the CLI accepting it is
   debatable).
4. Such a case seems fringe and even a bug in whatever is calling the
   CLI with such options.

Signed-off-by: Brian Goff <cpuguy83@gmail.com>
Copy link
Member

@thaJeztah thaJeztah 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 thaJeztah merged commit e0c87f9 into moby:master Mar 18, 2021
@cpuguy83 cpuguy83 deleted the TestDockerNetworkFlagAlias branch March 19, 2021 22:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants