integration-cli: remove deprecated `dockerCmd` and `waitRun` utilities by thaJeztah · Pull Request #46088 · moby/moby

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


 	// 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


 	// 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

 	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 :)

Signed-off-by: Sebastiaan van Stijn <[email protected]>

Also adding some consts for fixed values. Signed-off-by: Sebastiaan van Stijn <[email protected]>

Signed-off-by: Sebastiaan van Stijn <[email protected]>

Also renaming vars that collided with package-level vars and using consts for fixed values. Signed-off-by: Sebastiaan van Stijn <[email protected]>

Signed-off-by: Sebastiaan van Stijn <[email protected]>

Also adding some consts for fixed values. Signed-off-by: Sebastiaan van Stijn <[email protected]>

Signed-off-by: Sebastiaan van Stijn <[email protected]>

Also fixed some variables that shadowed package-level vars, and used consts for fixed values. Signed-off-by: Sebastiaan van Stijn <[email protected]>

Signed-off-by: Sebastiaan van Stijn <[email protected]>

thaJeztah · 2023-10-19T16:03:33Z

👍 updated; addressed the comments (❤️)

thaJeztah added status/2-code-review area/testing kind/refactor PR's that refactor, or clean-up code labels Jul 27, 2023

thaJeztah force-pushed the remove_deprecated_utils branch from 93355ac to 23c7d4c Compare July 27, 2023 13:40

This comment was marked as resolved.

Sign in to view

thaJeztah commented Jul 27, 2023

View reviewed changes

integration-cli/docker_cli_events_test.go Outdated Show resolved Hide resolved

thaJeztah force-pushed the remove_deprecated_utils branch 2 times, most recently from 985e00b to 00be3f1 Compare July 27, 2023 15:39

thaJeztah marked this pull request as ready for review July 27, 2023 19:16

thaJeztah force-pushed the remove_deprecated_utils branch from dbe6b79 to de03113 Compare July 27, 2023 22:33

thaJeztah marked this pull request as draft July 27, 2023 23:19

thaJeztah force-pushed the remove_deprecated_utils branch 2 times, most recently from 3940c56 to 3d26f90 Compare July 28, 2023 09:03

thaJeztah changed the title ~~integration-cli: remove various uses of deprecated dockerCmd and waitRun~~ integration-cli: remove deprecated dockerCmd and waitRun utilities Jul 28, 2023

thaJeztah marked this pull request as ready for review July 28, 2023 11:00

thaJeztah marked this pull request as draft July 30, 2023 13:56

thaJeztah force-pushed the remove_deprecated_utils branch from 3d26f90 to 8516a9f Compare August 14, 2023 15:44

thaJeztah force-pushed the remove_deprecated_utils branch from 8516a9f to 0748c6f Compare September 7, 2023 22:21

thaJeztah marked this pull request as ready for review September 7, 2023 22:22

thaJeztah mentioned this pull request Sep 7, 2023

Add otel tracing #45652

Merged

thaJeztah force-pushed the remove_deprecated_utils branch from 0748c6f to 07dab91 Compare October 11, 2023 12:27

thaJeztah force-pushed the remove_deprecated_utils branch from 07dab91 to 107b7c7 Compare October 18, 2023 07:40

rumpl reviewed Oct 18, 2023

View reviewed changes

integration-cli/docker_cli_build_test.go Outdated Show resolved Hide resolved

rumpl reviewed Oct 18, 2023

View reviewed changes

integration-cli/docker_cli_images_test.go Show resolved Hide resolved

rumpl approved these changes Oct 18, 2023

View reviewed changes

laurazard approved these changes Oct 19, 2023

View reviewed changes

thaJeztah added 24 commits October 19, 2023 17:42

integration-cli: DockerCLISaveLoadSuite: replace dockerCmd

4292f42

Signed-off-by: Sebastiaan van Stijn <[email protected]>

integration-cli: DockerCLIBuildSuite: replace dockerCmd

980a5bc

Also adding some consts for fixed values. Signed-off-by: Sebastiaan van Stijn <[email protected]>

integration-cli: DockerCLICpSuite: replace dockerCmd

c867ac4

Signed-off-by: Sebastiaan van Stijn <[email protected]>

integration-cli: DockerRegistrySuite: replace dockerCmd

89d7b77

Also renaming vars that collided with package-level vars and using consts for fixed values. Signed-off-by: Sebastiaan van Stijn <[email protected]>

integration-cli: DockerCLISearchSuite: replace dockerCmd

16e4bc9

Signed-off-by: Sebastiaan van Stijn <[email protected]>

integration-cli: DockerCLIRmiSuite: replace dockerCmd

ba4d491

Signed-off-by: Sebastiaan van Stijn <[email protected]>

integration-cli: DockerCLIPortSuite: replace dockerCmd and waitRun

f1102e9

Signed-off-by: Sebastiaan van Stijn <[email protected]>

integration-cli: DockerCLIImagesSuite: replace dockerCmd

f482857

Signed-off-by: Sebastiaan van Stijn <[email protected]>

integration-cli: DockerCLIVolumeSuite: replace dockerCmd

9c161cc

Signed-off-by: Sebastiaan van Stijn <[email protected]>

integration-cli: DockerDaemonSuite: replace dockerCmd

d1108b8

Signed-off-by: Sebastiaan van Stijn <[email protected]>

integration-cli: DockerCLIUpdateSuite: replace dockerCmd and waitRun

f7ddfdb

Also adding some consts for fixed values. Signed-off-by: Sebastiaan van Stijn <[email protected]>

integration-cli: DockerCLIStartSuite: replace dockerCmd

a4522b6

Signed-off-by: Sebastiaan van Stijn <[email protected]>

integration-cli: DockerCLICommitSuite: replace dockerCmd

7d2a6f2

Signed-off-by: Sebastiaan van Stijn <[email protected]>

integration-cli: DockerCLIStatsSuite: replace dockerCmd and waitRun

5c297fc

Signed-off-by: Sebastiaan van Stijn <[email protected]>

integration-cli: DockerCLICreateSuite: replace dockerCmd and waitRun

c388416

Signed-off-by: Sebastiaan van Stijn <[email protected]>

integration-cli: DockerCLIRestartSuite: replace dockerCmd and waitRun

268e72f

Signed-off-by: Sebastiaan van Stijn <[email protected]>

integration-cli: DockerCLIHistorySuite: replace dockerCmd

6ce6b63

Signed-off-by: Sebastiaan van Stijn <[email protected]>

integration-cli: DockerCLITopSuite: replace dockerCmd

885eceb

Signed-off-by: Sebastiaan van Stijn <[email protected]>

integration-cli: DockerCLIImportSuite: replace dockerCmd

5f88447

Signed-off-by: Sebastiaan van Stijn <[email protected]>

integration-cli: DockerExternalVolumeSuite: replace dockerCmd

a837cb4

Signed-off-by: Sebastiaan van Stijn <[email protected]>

integration-cli: DockerCLIPluginLogDriverSuite: replace dockerCmd

e28da76

Signed-off-by: Sebastiaan van Stijn <[email protected]>

integration-cli: DockerCLIPluginsSuite: replace dockerCmd

285ab8c

Also fixed some variables that shadowed package-level vars, and used consts for fixed values. Signed-off-by: Sebastiaan van Stijn <[email protected]>

integration-cli: DockerCLIPruneSuite: replace dockerCmd

06115bf

Signed-off-by: Sebastiaan van Stijn <[email protected]>

integration-cli: remove deprecated dockerCmd and waitRun utilities

5a72ed3

Signed-off-by: Sebastiaan van Stijn <[email protected]>

thaJeztah force-pushed the remove_deprecated_utils branch from 107b7c7 to 5a72ed3 Compare October 19, 2023 16:02

thaJeztah added this to the 25.0.0 milestone Oct 19, 2023

laurazard approved these changes Oct 20, 2023

View reviewed changes

thaJeztah merged commit c3ca4f5 into moby:master Oct 20, 2023

thaJeztah deleted the remove_deprecated_utils branch October 20, 2023 13:44

Comments

Conversation

thaJeztah commented Jul 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as resolved.

Uh oh!

thaJeztah commented Jul 30, 2023

Uh oh!

thaJeztah commented Sep 7, 2023

Uh oh!

thaJeztah commented Oct 11, 2023

Uh oh!

Uh oh!

Uh oh!

rumpl left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Oct 18, 2023

Uh oh!

laurazard left a comment

Choose a reason for hiding this comment

Uh oh!

laurazard Oct 18, 2023

Choose a reason for hiding this comment

Uh oh!

laurazard Oct 19, 2023

Choose a reason for hiding this comment

Uh oh!

thaJeztah Oct 19, 2023

Choose a reason for hiding this comment

Uh oh!

laurazard Oct 18, 2023

Choose a reason for hiding this comment

Uh oh!

thaJeztah Oct 19, 2023

Choose a reason for hiding this comment

Uh oh!

laurazard Oct 18, 2023

Choose a reason for hiding this comment

Uh oh!

laurazard Oct 19, 2023

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Oct 19, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

thaJeztah commented Jul 27, 2023 •

edited

Loading