From 6e18a44880533869a0b04a2e1ce5660c82cb1952 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Thu, 5 Dec 2024 09:16:25 +0100 Subject: [PATCH] Set new default approval mode based on repo visibility (#4456) Co-authored-by: Patrick Schratz Co-authored-by: Anbraten <6918444+anbraten@users.noreply.github.com> --- cli/repo/repo_update.go | 24 ++----- docs/src/pages/migrations.md | 1 + server/api/repo.go | 9 +-- server/pipeline/gated.go | 22 ++++--- .../019_gated_to_require_approval.go | 22 ++----- ...2_set-new-defaults-for-require_approval.go | 62 +++++++++++++++++++ server/store/datastore/migration/migration.go | 1 + woodpecker-go/woodpecker/types.go | 1 - 8 files changed, 91 insertions(+), 51 deletions(-) create mode 100644 server/store/datastore/migration/022_set-new-defaults-for-require_approval.go diff --git a/cli/repo/repo_update.go b/cli/repo/repo_update.go index b6a2e5846..d8bc73917 100644 --- a/cli/repo/repo_update.go +++ b/cli/repo/repo_update.go @@ -36,8 +36,8 @@ var repoUpdateCmd = &cli.Command{ Usage: "repository is trusted", }, &cli.BoolFlag{ - Name: "gated", - Usage: "repository is gated", + Name: "gated", // TODO: remove in next release + Hidden: true, }, &cli.StringFlag{ Name: "require-approval", @@ -82,7 +82,6 @@ func repoUpdate(ctx context.Context, c *cli.Command) error { config = c.String("config") timeout = c.Duration("timeout") trusted = c.Bool("trusted") - gated = c.Bool("gated") requireApproval = c.String("require-approval") pipelineCounter = int(c.Int("pipeline-counter")) unsafe = c.Bool("unsafe") @@ -92,29 +91,18 @@ func repoUpdate(ctx context.Context, c *cli.Command) error { if c.IsSet("trusted") { patch.IsTrusted = &trusted } - // TODO: remove isGated in next major release + + // TODO: remove in next release if c.IsSet("gated") { - if gated { - patch.RequireApproval = &woodpecker.RequireApprovalAllEvents - } else { - patch.RequireApproval = &woodpecker.RequireApprovalNone - } + return fmt.Errorf("'gated' option has been set in version 2.8, use 'require-approval' in >= 3.0") } + if c.IsSet("require-approval") { if mode := woodpecker.ApprovalMode(requireApproval); mode.Valid() { patch.RequireApproval = &mode } else { return fmt.Errorf("update approval mode failed: '%s' is no valid mode", mode) } - - // TODO: remove isGated in next major release - if requireApproval == string(woodpecker.RequireApprovalAllEvents) { - trueBool := true - patch.IsGated = &trueBool - } else if requireApproval == string(woodpecker.RequireApprovalNone) { - falseBool := false - patch.IsGated = &falseBool - } } if c.IsSet("timeout") { v := int64(timeout / time.Minute) diff --git a/docs/src/pages/migrations.md b/docs/src/pages/migrations.md index 009e77c70..e4bc3d227 100644 --- a/docs/src/pages/migrations.md +++ b/docs/src/pages/migrations.md @@ -10,6 +10,7 @@ This will be the next version of Woodpecker. ## User migrations +- `gated` has been replaced by `require-approval` - Removed built-in environment variables: - `CI_COMMIT_URL` use `CI_PIPELINE_FORGE_URL` - `CI_STEP_FINISHED` as empty during execution diff --git a/server/api/repo.go b/server/api/repo.go index 23f3006b0..d5c8a77b4 100644 --- a/server/api/repo.go +++ b/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 diff --git a/server/pipeline/gated.go b/server/pipeline/gated.go index 38fe2a1e2..bd59122f9 100644 --- a/server/pipeline/gated.go +++ b/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 } diff --git a/server/store/datastore/migration/019_gated_to_require_approval.go b/server/store/datastore/migration/019_gated_to_require_approval.go index 2b669d6c6..d025d1e76 100644 --- a/server/store/datastore/migration/019_gated_to_require_approval.go +++ b/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 } diff --git a/server/store/datastore/migration/022_set-new-defaults-for-require_approval.go b/server/store/datastore/migration/022_set-new-defaults-for-require_approval.go new file mode 100644 index 000000000..b71709218 --- /dev/null +++ b/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 + }, +} diff --git a/server/store/datastore/migration/migration.go b/server/store/datastore/migration/migration.go index 8826f68f9..df0cd8280 100644 --- a/server/store/datastore/migration/migration.go +++ b/server/store/datastore/migration/migration.go @@ -50,6 +50,7 @@ var migrationTasks = []*xormigrate.Migration{ &gatedToRequireApproval, &removeRepoNetrcOnlyTrusted, &renameTokenFields, + &setNewDefaultsForRequireApproval, } var allBeans = []any{ diff --git a/woodpecker-go/woodpecker/types.go b/woodpecker-go/woodpecker/types.go index 4a560af97..3e36b1dfc 100644 --- a/woodpecker-go/woodpecker/types.go +++ b/woodpecker-go/woodpecker/types.go @@ -80,7 +80,6 @@ type ( RepoPatch struct { Config *string `json:"config_file,omitempty"` IsTrusted *bool `json:"trusted,omitempty"` - IsGated *bool `json:"gated,omitempty"` // TODO: remove in next major release RequireApproval *ApprovalMode `json:"require_approval,omitempty"` Timeout *int64 `json:"timeout,omitempty"` Visibility *string `json:"visibility"`