integration-cli: remove deprecated `dockerCmd` and `waitRun` utilities #46088

thaJeztah · 2023-07-27T11:40:23Z

depends on / to be rebased after Add otel tracing #45652

Dusting off some old branches 😂

integration-cli: remove transformCmd utility
integration-cli: DockerCLIPsSuite: replace dockerCmd and waitRun
integration-cli: DockerCLIInspectSuite: replace dockerCmd and waitRun
integration-cli: DockerNetworkSuite: replace dockerCmd and waitRun
integration-cli: DockerCLINetmodeSuite: replace dockerCmd and waitRun
integration-cli: dockerCmdWithFail: remove unused return
integration-cli: DockerAPISuite: replace dockerCmd and waitRun
integration-cli: DockerCLIAttachSuite: replace dockerCmd and waitRun
integration-cli: DockerCLIEventSuite: replace dockerCmd and waitRun
integration-cli: DockerCLIExecSuite: replace dockerCmd and waitRun
integration-cli: DockerCLIInfoSuite: replace dockerCmd and waitRun
integration-cli: findContainerIP: replace dockerCmd
integration-cli: DockerBenchmarkSuite: replace dockerCmd and waitRun
integration-cli: DockerCLIHealthSuite: replace dockerCmd and waitRun
integration-cli: DockerCLILinksSuite: replace dockerCmd and waitRun
integration-cli: DockerRegistryAuthHtpasswdSuite: replace dockerCmd
integration-cli: DockerCLILogsSuite: replace dockerCmd and waitRun
integration-cli: DockerCLIRunSuite: replace dockerCmd and waitRun
integration-cli: DockerCLISaveLoadSuite: replace dockerCmd
integration-cli: DockerCLIBuildSuite: replace dockerCmd
integration-cli: DockerCLICpSuite: replace dockerCmd
integration-cli: DockerRegistrySuite: replace dockerCmd
integration-cli: replace dockerCmd
integration-cli: DockerCLISearchSuite: replace dockerCmd
integration-cli: DockerCLIRmiSuite: replace dockerCmd
integration-cli: DockerCLIPortSuite: replace dockerCmd and waitRun
integration-cli: DockerCLIImagesSuite: replace dockerCmd
integration-cli: DockerCLIVolumeSuite: replace dockerCmd
integration-cli: DockerDaemonSuite: replace dockerCmd
integration-cli: DockerCLIUpdateSuite: replace dockerCmd and waitRun
integration-cli: DockerCLIStartSuite: replace dockerCmd
integration-cli: DockerCLICommitSuite: replace dockerCmd
integration-cli: DockerCLIStatsSuite: replace dockerCmd and waitRun

- A picture of a cute animal (not mandatory but encouraged)

integration-cli/docker_cli_events_test.go

thaJeztah · 2023-07-30T13:56:07Z

I'll temporarily move this one to draft, pending #45652

I thought it would only conflict on imports, but after reviewing that PR, I noticed it also changes some lines around dockerCmd (and related), so I'll rebase this one after the other PR was merged.

/cc @cpuguy83 (reviews of this one welcome 🤗)

thaJeztah · 2023-09-07T23:22:29Z

@rumpl @vvoland @cpuguy83 ptal 🤗

thaJeztah · 2023-10-11T16:06:20Z

Unrelated, but interesting failure;

=== FAIL: amd64.integration.system TestLoginFailsWithBadCredentials (1.17s)
    login_test.go:28: assertion failed: expected error to contain "unauthorized: incorrect username or password", got "Error response from daemon: login attempt to https://registry-1.docker.io/v2/ failed with status: 401 Unauthorized"
        Error response from daemon: login attempt to https://registry-1.docker.io/v2/ failed with status: 401 Unauthorized

integration-cli/docker_cli_build_test.go

integration-cli/docker_cli_images_test.go

rumpl

LGTM but I'll be honest I half reviewed this, my eyes hurt. It's fine to merge, most of the changes are mechanical

thaJeztah · 2023-10-18T11:17:01Z

Yeah, it's a pain to review this one; MOST of the changes are mechanical; there are some cases where I made some small "somewhat" changes (e.g. remove intermediate vars while updating, or (as pointed out in a comment above) changed vars to consts. Most of those were because I saw touching those lines anyway, so might as well update to reduce follow-up code-churn.

It's been a while since I made this PR, but I recall there were some slightly opinionated changes; e.g. some "utility" lines (e.g. create a container and get its ID from the output) where I swapped CombinedOutput for StdOut. For those, I mostly made the assumption that those commands themselves are not part of the "coverage" of the test, so if they ran successfully we could just consume the expected output (disregarding output on StdErr, if any).

laurazard

LGTM, but ouch, my eyes 😅

laurazard · 2023-10-18T14:43:05Z

integration-cli/docker_api_attach_test.go

@@ -136,7 +137,7 @@ func (s *DockerAPISuite) TestPostContainersAttach(c *testing.T) {
 	}

 	// Create a container that only emits stdout.
-	cid, _ := dockerCmd(c, "run", "-di", "busybox", "cat")
+	cid := cli.DockerCmd(c, "run", "-di", "busybox", "cat").Combined()


Why use .Combined() here
(see next comment)

(Wrote this pair of comment right when I was starting to review this, but after looking at everything I think you did make the changes to use .Stdout() when it makes sense, I think this is the only odd one out.

Ah, yes, this one definitely can be StdOut() because we're capturing the output of the CLI (container ID) here, not so much the output of the container's command.

laurazard · 2023-10-18T14:44:11Z

integration-cli/docker_api_attach_test.go

@@ -152,7 +153,7 @@ func (s *DockerAPISuite) TestPostContainersAttach(c *testing.T) {
 	expectTimeout(wc, br, "stdout")

 	// Test the similar functions of the stderr stream.
-	cid, _ = dockerCmd(c, "run", "-di", "busybox", "/bin/sh", "-c", "cat >&2")
+	cid = cli.DockerCmd(c, "run", "-di", "busybox", "/bin/sh", "-c", "cat >&2").Stdout()


But then use .Stdout() here? They're both doing the same thing, just grabbing the container ID to use later, right?

If this was a 1:1 change we should just use .Combined() everywhere, but if we're making changes that make sense then we could use .Stdout() for both of these, right?

Yeah, I guess I got confused and thought we could be capturing the container's process output (or maybe I just got fatigued 😂).

laurazard · 2023-10-18T15:40:11Z

integration-cli/docker_cli_save_load_unix_test.go

-	repoName := "foobar-save-load-test"
-	before, _ := dockerCmd(c, "commit", name, repoName)
+	imgRepoName := "foobar-save-load-test"
+	before := cli.DockerCmd(c, "commit", name, imgRepoName).Combined()


This can probably be a .Stdout(), right? docker commit prints to stdout if everything went okay (which seems to be expected here)

laurazard · 2023-10-19T11:34:45Z

integration-cli/docker_cli_cp_test.go


 	// It *should not* have copied the directory using the target's name, but
 	// used the given name instead.
 	unexpectedPath := filepath.Join(testVol, cpTestPathParent)
 	stat, err := os.Lstat(unexpectedPath)
 	if err == nil {
-		out = fmt.Sprintf("target name was copied: %q - %q", stat.Mode(), stat.Name())
+		c.Errorf("target name was unexpectedly preserved: %q - %q", stat.Mode(), stat.Name())
 	}


This is much more readable :)