Set new default approval mode based on repo visibility (#4456)

Co-authored-by: Patrick Schratz <patrick.schratz@gmail.com>
Co-authored-by: Anbraten <6918444+anbraten@users.noreply.github.com>

server/api/repo.go

@@ -258,12 +258,9 @@ func PatchRepo(c *gin.Context) {
 			c.String(http.StatusBadRequest, "Invalid require-approval setting")
 			return
 		}
-	} else if in.IsGated != nil { // TODO: remove isGated in next major release
-		if *in.IsGated {
-			repo.RequireApproval = model.RequireApprovalAllEvents
-		} else {
-			repo.RequireApproval = model.RequireApprovalForks
-		}
+	} else if in.IsGated != nil {
+		c.String(http.StatusBadRequest, "'gated' option has been removed, use 'require-approval' in >= 3.0")
+		return
 	}
 	if in.Timeout != nil {
 		repo.Timeout = *in.Timeout

server/pipeline/gated.go

@@ -31,23 +31,25 @@ func needsApproval(repo *model.Repo, pipeline *model.Pipeline) bool {
 		return false
 	}
 
+	switch repo.RequireApproval {
 	// repository allows all events without approval
-	if repo.RequireApproval == model.RequireApprovalNone {
+	case model.RequireApprovalNone:
 		return false
-	}
 
 	// repository requires approval for pull requests from forks
-	if pipeline.Event == model.EventPull && pipeline.FromFork {
-		return true
-	}
+	case model.RequireApprovalForks:
+		if pipeline.Event == model.EventPull && pipeline.FromFork {
+			return true
+		}
 
 	// repository requires approval for pull requests
-	if pipeline.Event == model.EventPull && repo.RequireApproval == model.RequireApprovalPullRequests {
-		return true
-	}
+	case model.RequireApprovalPullRequests:
+		if pipeline.Event == model.EventPull {
+			return true
+		}
 
-	// repository requires approval for all events
-	if repo.RequireApproval == model.RequireApprovalAllEvents {
+		// repository requires approval for all events
+	case model.RequireApprovalAllEvents:
 		return true
 	}
 

server/store/datastore/migration/019_gated_to_require_approval.go

@@ -26,10 +26,8 @@ var gatedToRequireApproval = xormigrate.Migration{
 	ID: "gated-to-require-approval",
 	MigrateSession: func(sess *xorm.Session) (err error) {
 		const (
-			RequireApprovalNone         string = "none"
-			RequireApprovalForks        string = "forks"
-			RequireApprovalPullRequests string = "pull_requests"
-			RequireApprovalAllEvents    string = "all_events"
+			requireApprovalOldNotGated string = "old_not_gated"
+			requireApprovalAllEvents   string = "all_events"
 		)
 
 		type repos struct {
@@ -45,25 +43,17 @@ var gatedToRequireApproval = xormigrate.Migration{
 
 		// migrate gated repos
 		if _, err := sess.Exec(
-			builder.Update(builder.Eq{"require_approval": RequireApprovalAllEvents}).
+			builder.Update(builder.Eq{"require_approval": requireApprovalAllEvents}).
 				From("repos").
 				Where(builder.Eq{"gated": true})); err != nil {
 			return err
 		}
 
-		// migrate public repos to new default require approval
+		// migrate non gated repos to old_not_gated (no approval required)
 		if _, err := sess.Exec(
-			builder.Update(builder.Eq{"require_approval": RequireApprovalForks}).
+			builder.Update(builder.Eq{"require_approval": requireApprovalOldNotGated}).
 				From("repos").
-				Where(builder.Eq{"gated": false, "visibility": "public"})); err != nil {
-			return err
-		}
-
-		// migrate private repos to new default require approval
-		if _, err := sess.Exec(
-			builder.Update(builder.Eq{"require_approval": RequireApprovalNone}).
-				From("repos").
-				Where(builder.Eq{"gated": false}.And(builder.Neq{"visibility": "public"}))); err != nil {
+				Where(builder.Eq{"gated": false})); err != nil {
 			return err
 		}
 

server/store/datastore/migration/022_set-new-defaults-for-require_approval.go

@@ -0,0 +1,62 @@
+// Copyright 2024 Woodpecker Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package migration
+
+import (
+	"fmt"
+
+	"src.techknowlogick.com/xormigrate"
+	"xorm.io/builder"
+	"xorm.io/xorm"
+)
+
+var setNewDefaultsForRequireApproval = xormigrate.Migration{
+	ID: "set-new-defaults-for-require-approval",
+	MigrateSession: func(sess *xorm.Session) (err error) {
+		const (
+			RequireApprovalOldNotGated string = "old_not_gated"
+			RequireApprovalNone        string = "none"
+			RequireApprovalForks       string = "forks"
+			RequireApprovalAllEvents   string = "all_events"
+		)
+
+		type repos struct {
+			RequireApproval string `xorm:"require_approval"`
+			Visibility      string `xorm:"varchar(10) 'visibility'"`
+		}
+
+		if err := sess.Sync(new(repos)); err != nil {
+			return fmt.Errorf("sync new models failed: %w", err)
+		}
+
+		// migrate public repos to require approval for forks
+		if _, err := sess.Exec(
+			builder.Update(builder.Eq{"require_approval": RequireApprovalForks}).
+				From("repos").
+				Where(builder.Eq{"require_approval": RequireApprovalOldNotGated, "visibility": "public"})); err != nil {
+			return err
+		}
+
+		// migrate private repos to require no approval
+		if _, err := sess.Exec(
+			builder.Update(builder.Eq{"require_approval": RequireApprovalNone}).
+				From("repos").
+				Where(builder.Eq{"require_approval": RequireApprovalOldNotGated}.And(builder.Neq{"visibility": "public"}))); err != nil {
+			return err
+		}
+
+		return nil
+	},
+}

server/store/datastore/migration/migration.go

@@ -50,6 +50,7 @@ var migrationTasks = []*xormigrate.Migration{
 	&gatedToRequireApproval,
 	&removeRepoNetrcOnlyTrusted,
 	&renameTokenFields,
+	&setNewDefaultsForRequireApproval,
 }
 
 var allBeans = []any{