chore: remove redundant CreateRun magic string (#555)

@fsaintjacques A heads up - I recall removing the magic string behaviour
from your software, so I'm pretty sure this is now safe to remove from
OTF.

(If you recall, when creating a run, we wanted the run to automatically
fetch the config from the git repo, so we introduced a magic string to
invoke this behaviour. But then I realised you could simply leave out
the configuration version ID from CreateRun and that invokes the same
behaviour).

internal/http/html/static/templates/content/workspace_get.tmpl

@@ -31,7 +31,6 @@
         <div class="actions-container">
           <h3 class="font-semibold mb-2">Actions</h3>
           <form id="workspace-start-run-form" action="{{ startRunWorkspacePath .Workspace.ID }}" method="POST">
-            <input type="hidden" name="connected" value="{{ ne .Workspace.Connection nil }}">
             <select name="operation" id="start-run-operation" onchange="this.form.submit()">
               <option value="" selected>-- start run --</option>
               <option value="plan-only">plan only</option>

internal/integration/run_api_test.go

@@ -35,50 +35,6 @@ func TestIntegration_RunAPI(t *testing.T) {
 	})
 	require.NoError(t, err)
 
-	// test the "magic string" behaviour specific to OTF: if
-	// run.PullVCSMagicString is specified for the config version ID then the
-	// config is pulled from the workspace's connected repo.
-	t.Run("magic string - create run using config from repo", func(t *testing.T) {
-		vcsProvider := daemon.createVCSProvider(t, ctx, org)
-		ws, err := daemon.CreateWorkspace(ctx, workspace.CreateOptions{
-			Name:         internal.String("magic-connected-workspace"),
-			Organization: internal.String(org.Name),
-			ConnectOptions: &workspace.ConnectOptions{
-				RepoPath:      &repo,
-				VCSProviderID: &vcsProvider.ID,
-			},
-		})
-		require.NoError(t, err)
-
-		created, err := tfeClient.Runs.Create(ctx, tfe.RunCreateOptions{
-			ConfigurationVersion: &tfe.ConfigurationVersion{
-				ID: run.PullVCSMagicString,
-			},
-			Workspace: &tfe.Workspace{
-				ID: ws.ID,
-			},
-		})
-		require.NoError(t, err)
-
-		// wait for run to reach planned status
-		for event := range daemon.sub {
-			if r, ok := event.Payload.(*run.Run); ok {
-				switch r.Status {
-				case internal.RunErrored:
-					t.Fatal("run unexpectedly errored")
-				case internal.RunPlanned:
-					// run should have planned two resources (defined in the config from the
-					// github repo)
-					planned, err := daemon.GetRun(ctx, created.ID)
-					require.NoError(t, err)
-
-					assert.Equal(t, 2, planned.Plan.ResourceReport.Additions)
-					return // success
-				}
-			}
-		}
-	})
-
 	// pull config from workspace's vcs repo
 	t.Run("create run using config from repo", func(t *testing.T) {
 		vcsProvider := daemon.createVCSProvider(t, ctx, org)

internal/integration/run_test.go

@@ -33,34 +33,6 @@ func TestRun(t *testing.T) {
 		assert.Equal(t, user.Username, *run.CreatedBy)
 	})
 
-	// test the "magic string" behaviour specific to OTF: if
-	// run.PullVCSMagicString is specified for the config version ID then the
-	// config is pulled from the workspace's connected repo.
-	t.Run("magic string - create run using config from repo", func(t *testing.T) {
-		// setup daemon along with fake github repo
-		repo := cloud.NewTestRepo()
-		daemon, _, ctx := setup(t, nil,
-			github.WithRepo(repo),
-			github.WithArchive(testutils.ReadFile(t, "../testdata/github.tar.gz")),
-		)
-		org := daemon.createOrganization(t, ctx)
-		vcsProvider := daemon.createVCSProvider(t, ctx, org)
-		ws, err := daemon.CreateWorkspace(ctx, workspace.CreateOptions{
-			Name:         internal.String("connected-workspace"),
-			Organization: internal.String(org.Name),
-			ConnectOptions: &workspace.ConnectOptions{
-				RepoPath:      &repo,
-				VCSProviderID: &vcsProvider.ID,
-			},
-		})
-		require.NoError(t, err)
-
-		_, err = daemon.CreateRun(ctx, ws.ID, run.RunCreateOptions{
-			ConfigurationVersionID: internal.String(run.PullVCSMagicString),
-		})
-		require.NoError(t, err)
-	})
-
 	t.Run("create run using config from repo", func(t *testing.T) {
 		// setup daemon along with fake github repo
 		repo := cloud.NewTestRepo()

internal/run/factory.go

@@ -23,25 +23,16 @@ func (f *factory) NewRun(ctx context.Context, workspaceID string, opts RunCreate
 		return nil, err
 	}
 
-	// TODO(@leg100): remove PullVCSMagicString because (c)(ii) triggers the
-	// same behaviour.
-	//
-	// There are three possibilities for the ConfigurationVersionID value:
-	// (a) equals PullVCSMagicString in which case a config version is first
-	// created from the contents of the vcs repo connected to the workspace
-	// (b) non-nil, in which case it is deemed to be a configuration version id
+	// There are two possibilities for the ConfigurationVersionID value:
+	// (a) non-nil, in which case it is deemed to be a configuration version id
 	// and an existing config version with that ID is retrieved.
-	// (c) nil, in which it depends whether the workspace is connected to a vcs
+	// (b) nil, in which it depends whether the workspace is connected to a vcs
 	// repo:
 	// 	(i) not connected: the latest config version is retrieved.
 	// 	(ii) connected: same behaviour as (a): vcs repo contents are retrieved.
 	var cv *configversion.ConfigurationVersion
 	if opts.ConfigurationVersionID != nil {
-		if *opts.ConfigurationVersionID == PullVCSMagicString {
-			cv, err = f.createConfigVersionFromVCS(ctx, ws)
-		} else {
-			cv, err = f.GetConfigurationVersion(ctx, *opts.ConfigurationVersionID)
-		}
+		cv, err = f.GetConfigurationVersion(ctx, *opts.ConfigurationVersionID)
 	} else if ws.Connection == nil {
 		cv, err = f.GetLatestConfigurationVersion(ctx, workspaceID)
 	} else {
@@ -57,9 +48,6 @@ func (f *factory) NewRun(ctx context.Context, workspaceID string, opts RunCreate
 // createConfigVersionFromVCS creates a config version from the vcs repo
 // connected to the workspace using the contents of the vcs repo.
 func (f *factory) createConfigVersionFromVCS(ctx context.Context, ws *workspace.Workspace) (*configversion.ConfigurationVersion, error) {
-	if ws.Connection == nil {
-		return nil, workspace.ErrNoVCSConnection
-	}
 	client, err := f.GetVCSClient(ctx, ws.Connection.VCSProviderID)
 	if err != nil {
 		return nil, err

internal/run/factory_test.go

@@ -82,24 +82,6 @@ func TestFactory(t *testing.T) {
 		assert.True(t, got.AutoApply)
 	})
 
-	t.Run("magic string - pull from vcs", func(t *testing.T) {
-		f := newTestFactory(
-			&workspace.Workspace{
-				Connection: &workspace.Connection{},
-			},
-			&configversion.ConfigurationVersion{},
-		)
-
-		got, err := f.NewRun(ctx, "", RunCreateOptions{
-			ConfigurationVersionID: internal.String(PullVCSMagicString),
-		})
-		require.NoError(t, err)
-
-		// fake config version service sets the config version ID to "created"
-		// if it was newly created
-		assert.Equal(t, "created", got.ConfigurationVersionID)
-	})
-
 	t.Run("pull from vcs", func(t *testing.T) {
 		f := newTestFactory(
 			&workspace.Workspace{
@@ -115,18 +97,6 @@ func TestFactory(t *testing.T) {
 		// if it was newly created
 		assert.Equal(t, "created", got.ConfigurationVersionID)
 	})
-
-	t.Run("pull from vcs workspace not connected error", func(t *testing.T) {
-		f := newTestFactory(
-			&workspace.Workspace{}, // workspace with no connection
-			&configversion.ConfigurationVersion{},
-		)
-
-		_, err := f.NewRun(ctx, "", RunCreateOptions{
-			ConfigurationVersionID: internal.String(PullVCSMagicString),
-		})
-		require.Equal(t, err, workspace.ErrNoVCSConnection)
-	})
 }
 
 type (

internal/run/run.go

@@ -20,12 +20,6 @@ const (
 	PlanFormatBinary = "bin"  // plan file in binary format
 	PlanFormatJSON   = "json" // plan file in json format
 
-	// When specified in place of a configuration version ID passed to
-	// RunCreateOptions this magic string instructs the run factory to
-	// automatically create a configuration version from the workspace connected
-	// vcs repo.
-	PullVCSMagicString = "__pull_vcs__"
-
 	PlanOnlyOperation     Operation = "plan-only"
 	PlanAndApplyOperation Operation = "plan-and-apply"
 	DestroyAllOperation   Operation = "destroy-all"
@@ -95,14 +89,8 @@ type (
 		RefreshOnly *bool
 		Message     *string
 		// Specifies the configuration version to use for this run. If the
-		// configuration version object is omitted, the run will be created using the
+		// configuration version ID is nil, the run will be created using the
 		// workspace's latest configuration version.
-		//
-		// Alternatively, if PullVCSMagicString is specified, and the workspace
-		// is connected to a vcs repo, then a configuration version is
-		// automatically created from the vcs repo and the run uses that
-		// configuration version. If the workspace is not connected to a
-		// workspace then an error is returned.
 		ConfigurationVersionID *string
 		TargetAddrs            []string
 		ReplaceAddrs           []string

internal/run/web.go

@@ -55,24 +55,16 @@ func (h *webHandlers) createRun(w http.ResponseWriter, r *http.Request) {
 	var params struct {
 		WorkspaceID string    `schema:"workspace_id,required"`
 		Operation   Operation `schema:"operation,required"`
-		Connected   bool      `schema:"connected,required"`
 	}
 	if err := decode.All(&params, r); err != nil {
 		h.Error(w, err.Error(), http.StatusUnprocessableEntity)
 		return
 	}
 
-	opts := RunCreateOptions{
+	run, err := h.svc.CreateRun(r.Context(), params.WorkspaceID, RunCreateOptions{
 		IsDestroy: internal.Bool(params.Operation == DestroyAllOperation),
 		PlanOnly:  internal.Bool(params.Operation == PlanOnlyOperation),
-	}
-	// if run's workspace is connected to a VCS repo then pull config from
-	// the repo
-	if params.Connected {
-		opts.ConfigurationVersionID = internal.String(PullVCSMagicString)
-	}
-
-	run, err := h.svc.CreateRun(r.Context(), params.WorkspaceID, opts)
+	})
 	if err != nil {
 		html.FlashError(w, err.Error())
 		http.Redirect(w, r, paths.Workspace(params.WorkspaceID), http.StatusFound)

internal/workspace/workspace.go

@@ -26,7 +26,6 @@ const (
 )
 
 var (
-	ErrNoVCSConnection                 = errors.New("workspace is not connected to a vcs repo")
 	ErrTagsRegexAndTriggerPatterns     = errors.New("cannot specify both tags-regex and trigger-patterns")
 	ErrTagsRegexAndAlwaysTrigger       = errors.New("cannot specify both tags-regex and always-trigger")
 	ErrTriggerPatternsAndAlwaysTrigger = errors.New("cannot specify both trigger-patterns and always-trigger")