roachtest: replace roachtest run --dry-run

[andreimatei]

... with a dedicated roachtest list.
The dryrun argument was polluting and indenting the code.

Release note: None

[andreimatei]

The first commit is #30146.

[cockroach-teamcity]

This change is 

[petermattis]

This unfortunately removes functionality: roachtest run -n vs roachtest bench -n.

Reviewed 4 of 4 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/cmd/roachtest/main.go, line 43 at r2 (raw file):

				return fmt.Errorf("cannot specify both an existing cluster (%s) and --local", clusterName)
			}
			if !dryrun {

This was useful, though, as it prevented trying to find the roachprod and workload binaries if you weren't running a test. I think you'll want to refactor where initBinaries is performed so that it is only done if we're actually running tests.

[andreimatei]

roachtest bench... who knew. Added roachtest list --bench.

Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/cmd/roachtest/main.go, line 43 at r2 (raw file):

Previously, petermattis (Peter Mattis) wrote…

This was useful, though, as it prevented trying to find the roachprod and workload binaries if you weren't running a test. I think you'll want to refactor where initBinaries is performed so that it is only done if we're actually running tests.

good catch. I left it here, but made it conditional on the command.

[petermattis]

I think it is good that you got rid of the dryrun variable infecting the run implementation. But I'm less thrilled about the user ergonomics of list and list --bench. A --dry-run flag is reasonably common in tools. Did you consider leaving the flag in place, but changing the implementation to be similar to what you've implemented for listCmd? That is, the runCmd function would contain something like:

if dryrun {
	names := r.ListAll(args)
	for _, name := range names {
		fmt.Println(name)
	}
} else {
	os.Exit(r.Run(args))
}

Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/cmd/roachtest/main.go, line 48 at r4 (raw file):

				"store-gen": {},
			}
			if _, ok := needsBinaries[cmd.Name()]; ok {

Nit: I think this is cleaner as a switch:

switch cmd.Name() {
case "run", "bench", "store-gen":
  initBinaries()
}

[andreimatei]

I have considered it.
--dry-run may be common enough for simple tools, but I don't think roachtest qualifies (particularly as I'm trying to make it more complex). This tool should allocate a cluster pool, then run tests through a scheduler. Were a --dry-run to exist, it'd be unclear (to me as a user) what parts should run and what parts shouldn't: how would it interact with the --parallelism and --count flags, for example?
Also, through the run() decontamination, I've also changed the output format of list/--dry-run - it no longer prints stuff like [PASS] which is a big deal as far as expectations from this tool go.
Basically, to quote a living classic, to consider --dry-run making sense with run I have to squint so hard my eyes would be closed.

Having said all this, I'll change it if you insist.

Reviewable status: complete! 0 of 0 LGTMs obtained

[petermattis]

I don't feel strongly about --dry-run and you raise good points. Another approach would be to replace --dry-run with --list. That is run --list, bench --list, etc. Feel free to ignore this suggestion.

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale)

[andreimatei]

I... still prefer a top-level command (particularly as you have a good suggestion elsewhere about also optionally printing the cluster spec for each test, which I might do).

Merging on Monday.

Reviewable status: complete! 1 of 0 LGTMs obtained

---

pkg/cmd/roachtest/main.go, line 48 at r4 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Nit: I think this is cleaner as a switch:

switch cmd.Name() {
case "run", "bench", "store-gen":
  initBinaries()
}

done

[andreimatei]

bors r+

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale)

[craig]

Build succeeded

• GitHub CI (Cockroach)