Finish port to cobra #1792

gsoltis · 2022-08-26T22:14:47Z

Ports root command execution to cobra.

Introduces a couple of abstractions:
cmdutil.Helper -> Holds common arguments to all commands
cmdutil.CmdBase -> Holds subcomponents common to all commands, possibly driven by the values in cmdutil.Helper. Examples include UI, Logger, and the API client

Port all of our turbo commands to be subcommands of a root command.

Reviewers: individual commits won't build, but group related functionality.

Fixes #759
Fixes #512

vercel · 2022-08-26T22:14:50Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated
turbo-site	✅ Ready (Inspect)	Visit Preview	Sep 12, 2022 at 4:58PM (UTC)

We don't have a `turbo version` command, so don't special case it in argument parsing. Fixes #1839 This is superceded by #1792

chris-olszewski

LGTM, a lot less plumbing

nathanhammond

Mostly comments and questions!

cli/cmd/turbo/main.go

cli/internal/cache/cache.go

nathanhammond · 2022-09-13T04:39:07Z

cli/internal/cache/cache.go

 	// skipping remote caching not currently a flag
 	flags.BoolVar(&opts.SkipFilesystem, "remote-only", false, _remoteOnlyHelp)
-	fs.AbsolutePathVar(flags, &opts.Dir, "cache-dir", repoRoot, "Specify local filesystem cache directory.", "./node_modules/.cache/turbo")
+	flags.StringVar(&opts.OverrideDir, "cache-dir", "", "Override the filesystem cache directory.")
+	flags.IntVar(&opts.Workers, "cache-workers", 10, "Set the number of concurrent cache operations")


(Not related to this PR.)

This is a bit weird to me. I feel like this should be bounded by the number of task threads instead of having separate configuration. In theory we should complete cache writing before yielding to the next task (assured correct), or at least before yielding to the next task which specifies colliding outputs (optimistically correct).

(In a new world where you write to a single tar this is even stranger.)

cli/internal/cache/cache.go

nathanhammond · 2022-09-13T04:41:58Z

cli/internal/cache/cache_http_test.go

+func (sr *errorResp) GetTeamID() string {
+	return ""
+}
+


TIL you can do stubbing in _test.go files the same way the _unix.go stuff works.

I assume that this works combinatorically too!

nathanhammond · 2022-09-13T08:11:57Z

cli/internal/cmdutil/cmdutil.go

+// RegisterCleanup saves a function to be run after turbo execution,
+// even if the command that runs returns an error
+func (h *Helper) RegisterCleanup(cleanup io.Closer) {
+	h.cleanups = append(h.cleanups, cleanup)


(change request)

We should mutex this or unexport it. Right now it can't cause an issue, but there isn't any guarantee that remains true. (Would need RegisterCleanup to be called in a separate goroutines and for it to get resized simultaneously, but ... possible, since everybody gets a reference.)

cli/internal/cmdutil/cmdutil.go

nathanhammond · 2022-09-13T11:18:34Z

cli/internal/cmdutil/cmdutil.go

+}
+
+// GetCmdBase returns a CmdBase instance configured with values from this helper.
+// It additionally returns a mechanism to set an error, so


You accidentally a sentence.

nathanhammond · 2022-09-13T11:21:00Z

cli/internal/cmdutil/cmdutil.go

+		vercelArtifactsToken := os.Getenv("VERCEL_ARTIFACTS_TOKEN")
+		vercelArtifactsOwner := os.Getenv("VERCEL_ARTIFACTS_OWNER")


@tknickman I'd love to think about how to make this configurable for any platform (e.g. if they implement their own remote cache).

nathanhammond · 2022-09-13T11:25:48Z

cli/internal/cmd/root.go

+	cmd.AddCommand(login.NewLinkCommand(helper))
+	cmd.AddCommand(login.NewLoginCommand(helper))
+	cmd.AddCommand(auth.LogoutCmd(helper))
+	cmd.AddCommand(auth.UnlinkCmd(helper))
+	cmd.AddCommand(info.BinCmd(helper))
+	cmd.AddCommand(daemon.GetCmd(helper, signalWatcher))
+	cmd.AddCommand(prune.GetCmd(helper))
+	cmd.AddCommand(run.GetCmd(helper, signalWatcher))


We should align this at some point.

Addresses feedback on #1792 Adds a mutex around our list of cleanups to run.

gsoltis force-pushed the gsoltis/cobra_root branch from 887663c to f87e923 Compare August 30, 2022 22:52

vercel bot deployed to Preview August 30, 2022 22:53 View deployment

gsoltis force-pushed the gsoltis/cobra_root branch from 301386e to 6680323 Compare August 31, 2022 16:30

vercel bot deployed to Preview August 31, 2022 16:30 View deployment

gsoltis force-pushed the gsoltis/cobra_root branch from 6680323 to 10ab674 Compare August 31, 2022 18:21

vercel bot deployed to Preview August 31, 2022 18:22 View deployment

gsoltis force-pushed the gsoltis/cobra_root branch from 10ab674 to ade0846 Compare August 31, 2022 18:22

vercel bot deployed to Preview August 31, 2022 18:23 View deployment

gsoltis marked this pull request as ready for review August 31, 2022 18:25

gsoltis requested a review from a team as a code owner August 31, 2022 18:25

gsoltis requested review from nathanhammond and chris-olszewski August 31, 2022 18:25

gsoltis mentioned this pull request Sep 6, 2022

Don't special case version command #1870

Merged

kodiakhq bot pushed a commit that referenced this pull request Sep 6, 2022

Don't special case version command (#1870)

9c2eb01

We don't have a `turbo version` command, so don't special case it in argument parsing. Fixes #1839 This is superceded by #1792

Greg Soltis added 10 commits September 9, 2022 10:43

Add Helper and CmdBase

4e31bb7

Implement root command and RunWithArgs

0ea22f7

Drop old config, update config_file, drop AbsolutePathVar

18bb7d0

Update cache to not depend on config

921db03

Update api client to use a flag for preflight and expose TeamID

bda1e6d

Switch main to use RunWithArgs

c2cf510

Port auth-related commands to user CmdBase

422cf92

Port daemon commands to use CmdBase

1ffb0d1

Port run command to use CmdBase

ace4b18

Port prune command to use CmdBase

de3f31b

gsoltis force-pushed the gsoltis/cobra_root branch from ade0846 to de3f31b Compare September 9, 2022 17:54

vercel bot deployed to Preview September 9, 2022 17:54 View deployment

chris-olszewski approved these changes Sep 12, 2022

View reviewed changes

gsoltis added the pr: automerge Kodiak will merge these automatically after checks pass label Sep 12, 2022

Merge branch 'main' into gsoltis/cobra_root

c6fe3f0

vercel bot deployed to Preview September 12, 2022 16:58 View deployment

kodiakhq bot merged commit 8928b11 into main Sep 12, 2022

kodiakhq bot deleted the gsoltis/cobra_root branch September 12, 2022 17:30

nathanhammond reviewed Sep 13, 2022

View reviewed changes

gsoltis mentioned this pull request Sep 13, 2022

Add mutex around helper cleanups #1947

Merged

kodiakhq bot pushed a commit that referenced this pull request Sep 14, 2022

Add mutex around helper cleanups (#1947)

5dfbb24

Addresses feedback on #1792 Adds a mutex around our list of cleanups to run.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Finish port to cobra #1792

Finish port to cobra #1792

gsoltis commented Aug 26, 2022 •

edited

Loading

vercel bot commented Aug 26, 2022 •

edited

Loading

chris-olszewski left a comment

nathanhammond left a comment

nathanhammond Sep 13, 2022

nathanhammond Sep 13, 2022

nathanhammond Sep 13, 2022

nathanhammond Sep 13, 2022

nathanhammond Sep 13, 2022

nathanhammond Sep 13, 2022

		vercelArtifactsToken := os.Getenv("VERCEL_ARTIFACTS_TOKEN")
		vercelArtifactsOwner := os.Getenv("VERCEL_ARTIFACTS_OWNER")

Finish port to cobra #1792

Finish port to cobra #1792

Conversation

gsoltis commented Aug 26, 2022 • edited Loading

vercel bot commented Aug 26, 2022 • edited Loading

chris-olszewski left a comment

Choose a reason for hiding this comment

nathanhammond left a comment

Choose a reason for hiding this comment

nathanhammond Sep 13, 2022

Choose a reason for hiding this comment

nathanhammond Sep 13, 2022

Choose a reason for hiding this comment

nathanhammond Sep 13, 2022

Choose a reason for hiding this comment

nathanhammond Sep 13, 2022

Choose a reason for hiding this comment

nathanhammond Sep 13, 2022

Choose a reason for hiding this comment

nathanhammond Sep 13, 2022

Choose a reason for hiding this comment

gsoltis commented Aug 26, 2022 •

edited

Loading

vercel bot commented Aug 26, 2022 •

edited

Loading