feat: Disallow using legacy params with rich params (#5974)

mtojek · web-flow · commit 271d68c862fa · 2023-02-02T16:44:57.000+01:00
* feat: Disallow using legacy params with rich params

* Fix

* nolint
diff --git a/cli/create.go b/cli/create.go
@@ -194,6 +194,21 @@ type buildParameters struct {
 // Any missing params will be prompted to the user. It supports legacy and rich parameters.
 func prepWorkspaceBuild(cmd *cobra.Command, client *codersdk.Client, args prepWorkspaceBuildArgs) (*buildParameters, error) {
 	ctx := cmd.Context()
+
+	var useRichParameters bool
+	if len(args.ExistingRichParams) > 0 && len(args.RichParameterFile) > 0 {
+		useRichParameters = true
+	}
+
+	var useLegacyParameters bool
+	if len(args.ExistingParams) > 0 || len(args.ParameterFile) > 0 {
+		useLegacyParameters = true
+	}
+
+	if useRichParameters && useLegacyParameters {
+		return nil, xerrors.Errorf("Rich parameters can't be used together with legacy parameters.")
+	}
+
 	templateVersion, err := client.TemplateVersion(ctx, args.Template.ActiveVersionID)
 	if err != nil {
 		return nil, err
diff --git a/coderd/coderdtest/swaggerparser.go b/coderd/coderdtest/swaggerparser.go
@@ -260,7 +260,7 @@ func assertGoCommentFirst(t *testing.T, comment SwaggerComment) {
 		text := strings.TrimSpace(line.Text)
 
 		if inSwaggerBlock {
-			if !strings.HasPrefix(text, "// @") {
+			if !strings.HasPrefix(text, "// @") && !strings.HasPrefix(text, "// nolint:") {
 				assert.Fail(t, "Go function comment must be placed before swagger comments")
 				return
 			}
diff --git a/coderd/database/dbgen/generator.go b/coderd/database/dbgen/generator.go
@@ -9,13 +9,13 @@ import (
 	"testing"
 	"time"
 
-	"github.com/coder/coder/cryptorand"
-	"github.com/tabbed/pqtype"
-
-	"github.com/coder/coder/coderd/database"
 	"github.com/google/uuid"
 	"github.com/moby/moby/pkg/namesgenerator"
 	"github.com/stretchr/testify/require"
+	"github.com/tabbed/pqtype"
+
+	"github.com/coder/coder/coderd/database"
+	"github.com/coder/coder/cryptorand"
 )
 
 // All methods take in a 'seed' object. Any provided fields in the seed will be
diff --git a/coderd/workspacebuilds.go b/coderd/workspacebuilds.go
@@ -297,6 +297,7 @@ func (api *API) workspaceBuildByBuildNumber(rw http.ResponseWriter, r *http.Requ
 // @Param request body codersdk.CreateWorkspaceBuildRequest true "Create workspace build request"
 // @Success 200 {object} codersdk.WorkspaceBuild
 // @Router /workspaces/{workspace}/builds [post]
+// nolint:gocyclo
 func (api *API) postWorkspaceBuilds(rw http.ResponseWriter, r *http.Request) {
 	ctx := r.Context()
 	apiKey := httpmw.APIKey(r)
@@ -505,23 +506,34 @@ func (api *API) postWorkspaceBuilds(rw http.ResponseWriter, r *http.Request) {
 		}
 	}
 
+	legacyParameters, err := api.Database.ParameterValues(ctx, database.ParameterValuesParams{
+		Scopes:   []database.ParameterScope{database.ParameterScopeWorkspace},
+		ScopeIds: []uuid.UUID{workspace.ID},
+	})
+	if err != nil && !xerrors.Is(err, sql.ErrNoRows) {
+		httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
+			Message: "Error fetching previous legacy parameters.",
+			Detail:  err.Error(),
+		})
+		return
+	}
+
+	if len(legacyParameters) > 0 && len(parameters) > 0 {
+		httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
+			Message: "Rich parameters can't be used together with legacy parameters.",
+		})
+		return
+	}
+
 	var workspaceBuild database.WorkspaceBuild
 	var provisionerJob database.ProvisionerJob
 	// This must happen in a transaction to ensure history can be inserted, and
 	// the prior history can update it's "after" column to point at the new.
 	err = api.Database.InTx(func(db database.Store) error {
-		existing, err := db.ParameterValues(ctx, database.ParameterValuesParams{
-			Scopes:   []database.ParameterScope{database.ParameterScopeWorkspace},
-			ScopeIds: []uuid.UUID{workspace.ID},
-		})
-		if err != nil && !xerrors.Is(err, sql.ErrNoRows) {
-			return xerrors.Errorf("Fetch previous parameters: %w", err)
-		}
-
 		// Write/Update any new params
 		now := database.Now()
 		for _, param := range createBuild.ParameterValues {
-			for _, exists := range existing {
+			for _, exists := range legacyParameters {
 				// If the param exists, delete the old param before inserting the new one
 				if exists.Name == param.Name {
 					err = db.DeleteParameterValueByID(ctx, exists.ID)
diff --git a/provisionerd/runner/runner.go b/provisionerd/runner/runner.go
@@ -728,6 +728,11 @@ func (r *Runner) runTemplateImportProvisionWithRichParameters(ctx context.Contex
 				return nil, nil, xerrors.New(msgType.Complete.Error)
 			}
 
+			if len(msgType.Complete.Parameters) > 0 && len(values) > 0 {
+				r.logger.Info(context.Background(), "template uses rich parameters which can't be used together with legacy parameters")
+				return nil, nil, xerrors.Errorf("invalid use of rich parameters")
+			}
+
 			r.logger.Info(context.Background(), "parse dry-run provision successful",
 				slog.F("resource_count", len(msgType.Complete.Resources)),
 				slog.F("resources", msgType.Complete.Resources),

Original file line number	Diff line number	Diff line change
`@@ -260,7 +260,7 @@ func assertGoCommentFirst(t *testing.T, comment SwaggerComment) {`
`260`	`260`	`text := strings.TrimSpace(line.Text)`
`261`	`261`
`262`	`262`	`if inSwaggerBlock {`
`263`		`- if !strings.HasPrefix(text, "// @") {`
	`263`	`+ if !strings.HasPrefix(text, "// @") && !strings.HasPrefix(text, "// nolint:") {`
`264`	`264`	`assert.Fail(t, "Go function comment must be placed before swagger comments")`
`265`	`265`	`return`
`266`	`266`	`}`