From f2a309e6c81f9ce0de5346e49efbc98fbd20e54b Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Mon, 11 Dec 2023 23:55:10 +0800 Subject: [PATCH] Improve doctor cli behavior (#28422) 1. Do not sort the "checks" slice again and again when "Register", it just wastes CPU when the Gitea instance runs 2. If a check doesn't exist, tell the end user 3. Add some tests --- cmd/doctor.go | 51 ++++++++++++++++++---------------------- cmd/doctor_test.go | 33 ++++++++++++++++++++++++++ modules/doctor/doctor.go | 16 ++++++++----- 3 files changed, 66 insertions(+), 34 deletions(-) create mode 100644 cmd/doctor_test.go diff --git a/cmd/doctor.go b/cmd/doctor.go index d040a3af1c..4085d37332 100644 --- a/cmd/doctor.go +++ b/cmd/doctor.go @@ -14,6 +14,7 @@ import ( "code.gitea.io/gitea/models/db" "code.gitea.io/gitea/models/migrations" migrate_base "code.gitea.io/gitea/models/migrations/base" + "code.gitea.io/gitea/modules/container" "code.gitea.io/gitea/modules/doctor" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/setting" @@ -22,6 +23,19 @@ import ( "xorm.io/xorm" ) +// CmdDoctor represents the available doctor sub-command. +var CmdDoctor = &cli.Command{ + Name: "doctor", + Usage: "Diagnose and optionally fix problems", + Description: "A command to diagnose problems with the current Gitea instance according to the given configuration. Some problems can optionally be fixed by modifying the database or data storage.", + + Subcommands: []*cli.Command{ + cmdDoctorCheck, + cmdRecreateTable, + cmdDoctorConvert, + }, +} + var cmdDoctorCheck = &cli.Command{ Name: "check", Usage: "Diagnose and optionally fix problems", @@ -60,19 +74,6 @@ var cmdDoctorCheck = &cli.Command{ }, } -// CmdDoctor represents the available doctor sub-command. -var CmdDoctor = &cli.Command{ - Name: "doctor", - Usage: "Diagnose and optionally fix problems", - Description: "A command to diagnose problems with the current Gitea instance according to the given configuration. Some problems can optionally be fixed by modifying the database or data storage.", - - Subcommands: []*cli.Command{ - cmdDoctorCheck, - cmdRecreateTable, - cmdDoctorConvert, - }, -} - var cmdRecreateTable = &cli.Command{ Name: "recreate-table", Usage: "Recreate tables from XORM definitions and copy the data.", @@ -177,6 +178,7 @@ func runDoctorCheck(ctx *cli.Context) error { if ctx.IsSet("list") { w := tabwriter.NewWriter(os.Stdout, 0, 8, 1, '\t', 0) _, _ = w.Write([]byte("Default\tName\tTitle\n")) + doctor.SortChecks(doctor.Checks) for _, check := range doctor.Checks { if check.IsDefault { _, _ = w.Write([]byte{'*'}) @@ -192,26 +194,20 @@ func runDoctorCheck(ctx *cli.Context) error { var checks []*doctor.Check if ctx.Bool("all") { - checks = doctor.Checks + checks = make([]*doctor.Check, len(doctor.Checks)) + copy(checks, doctor.Checks) } else if ctx.IsSet("run") { addDefault := ctx.Bool("default") - names := ctx.StringSlice("run") - for i, name := range names { - names[i] = strings.ToLower(strings.TrimSpace(name)) - } - + runNamesSet := container.SetOf(ctx.StringSlice("run")...) for _, check := range doctor.Checks { - if addDefault && check.IsDefault { + if (addDefault && check.IsDefault) || runNamesSet.Contains(check.Name) { checks = append(checks, check) - continue - } - for _, name := range names { - if name == check.Name { - checks = append(checks, check) - break - } + runNamesSet.Remove(check.Name) } } + if len(runNamesSet) > 0 { + return fmt.Errorf("unknown checks: %q", strings.Join(runNamesSet.Values(), ",")) + } } else { for _, check := range doctor.Checks { if check.IsDefault { @@ -219,6 +215,5 @@ func runDoctorCheck(ctx *cli.Context) error { } } } - return doctor.RunChecks(stdCtx, colorize, ctx.Bool("fix"), checks) } diff --git a/cmd/doctor_test.go b/cmd/doctor_test.go new file mode 100644 index 0000000000..75376a567e --- /dev/null +++ b/cmd/doctor_test.go @@ -0,0 +1,33 @@ +// Copyright 2023 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package cmd + +import ( + "context" + "testing" + + "code.gitea.io/gitea/modules/doctor" + "code.gitea.io/gitea/modules/log" + + "github.com/stretchr/testify/assert" + "github.com/urfave/cli/v2" +) + +func TestDoctorRun(t *testing.T) { + doctor.Register(&doctor.Check{ + Title: "Test Check", + Name: "test-check", + Run: func(ctx context.Context, logger log.Logger, autofix bool) error { return nil }, + + SkipDatabaseInitialization: true, + }) + app := cli.NewApp() + app.Commands = []*cli.Command{cmdDoctorCheck} + err := app.Run([]string{"./gitea", "check", "--run", "test-check"}) + assert.NoError(t, err) + err = app.Run([]string{"./gitea", "check", "--run", "no-such"}) + assert.ErrorContains(t, err, `unknown checks: "no-such"`) + err = app.Run([]string{"./gitea", "check", "--run", "test-check,no-such"}) + assert.ErrorContains(t, err, `unknown checks: "no-such"`) +} diff --git a/modules/doctor/doctor.go b/modules/doctor/doctor.go index ceee322852..559f8e06da 100644 --- a/modules/doctor/doctor.go +++ b/modules/doctor/doctor.go @@ -79,6 +79,7 @@ var Checks []*Check // RunChecks runs the doctor checks for the provided list func RunChecks(ctx context.Context, colorize, autofix bool, checks []*Check) error { + SortChecks(checks) // the checks output logs by a special logger, they do not use the default logger logger := log.BaseLoggerToGeneralLogger(&doctorCheckLogger{colorize: colorize}) loggerStep := log.BaseLoggerToGeneralLogger(&doctorCheckStepLogger{colorize: colorize}) @@ -104,20 +105,23 @@ func RunChecks(ctx context.Context, colorize, autofix bool, checks []*Check) err logger.Info("OK") } } - logger.Info("\nAll done.") + logger.Info("\nAll done (checks: %d).", len(checks)) return nil } // Register registers a command with the list func Register(command *Check) { Checks = append(Checks, command) - sort.SliceStable(Checks, func(i, j int) bool { - if Checks[i].Priority == Checks[j].Priority { - return Checks[i].Name < Checks[j].Name +} + +func SortChecks(checks []*Check) { + sort.SliceStable(checks, func(i, j int) bool { + if checks[i].Priority == checks[j].Priority { + return checks[i].Name < checks[j].Name } - if Checks[i].Priority == 0 { + if checks[i].Priority == 0 { return false } - return Checks[i].Priority < Checks[j].Priority + return checks[i].Priority < checks[j].Priority }) }