diff --git a/.github/workflows/pull-compliance.yml b/.github/workflows/pull-compliance.yml index 9a1cc267dd1..be133fb70a3 100644 --- a/.github/workflows/pull-compliance.yml +++ b/.github/workflows/pull-compliance.yml @@ -24,8 +24,7 @@ jobs: with: lint-cache: "true" - run: make deps-backend deps-tools - - run: TAGS="bindata" make generate-go # lint-go also lints with "bindata" tags which requires "_bindata.go" - - run: make lint-backend + - run: TAGS="bindata" make generate-go lint-backend # lint-go can lint with "bindata" tags lint-on-demand: needs: files-changed diff --git a/Makefile b/Makefile index 893e8fe2316..0e260eb4ad1 100644 --- a/Makefile +++ b/Makefile @@ -451,7 +451,7 @@ $(GO_LICENSE_FILE): go.mod go.sum GO=$(GO) $(GO) run build/generate-go-licenses.go $(GO_LICENSE_FILE) .PHONY: test-integration -test-integration: +test-integration: $(EXECUTABLE) @# Use a compiled binary: testlogger forwards gitea logs to t.Log, so `go test -v` @# would flood output per passing test. testcache can't help these tests anyway — @# they mutate the work directory, so cache inputs change between runs. @@ -463,7 +463,7 @@ test-integration-compile: $(GO) test $(GOTEST_FLAGS) -tags '$(TAGS)' -c -o /dev/null gitea.dev/tests/integration .PHONY: test-integration\#% -test-integration\#%: +test-integration\#%: $(EXECUTABLE) $(GO) test $(GOTEST_FLAGS) -tags '$(TAGS)' -run $(subst .,/,$*) gitea.dev/tests/integration .PHONY: test-migration diff --git a/modules/test/utils.go b/modules/test/utils.go index b682f0762e5..514a9a1141b 100644 --- a/modules/test/utils.go +++ b/modules/test/utils.go @@ -18,6 +18,7 @@ import ( "strconv" "strings" "sync" + "time" "gitea.dev/modules/json" "gitea.dev/modules/util" @@ -160,29 +161,47 @@ type TestingT interface { TempDir() string } +var externalServiceCheckResult sync.Map + func ExternalServiceHTTP(t TestingT, envVarName, def string) string { t.Helper() - val := util.IfZero(os.Getenv(envVarName), def) - if val == "" { + extSvc := util.IfZero(os.Getenv(envVarName), def) + if extSvc == "" { if AllowSkipExternalService() { t.Skipf("skipping test because %s is not set", envVarName) } else { t.Fatalf("%s is not set, but skipping is not allowed in CI", envVarName) } } - // minio's endpoint is "host:port" pattern - testURL := util.Iif(strings.Contains(val, "://"), val, "http://"+val) - resp, err := http.Get(testURL) - if err != nil { - if AllowSkipExternalService() { - t.Skipf("skipping test because %s is not ready", val) - } else { - t.Fatalf("%s is not ready, but skipping is not allowed in CI", val) + + // only need to check once, if there are saved check result, just use it + lastCheckErrAny, lastCheckExists := externalServiceCheckResult.Load(extSvc) + var lastCheckErr error + if !lastCheckExists { + { + // minio's endpoint is "host:port" pattern + testURL := util.Iif(strings.Contains(extSvc, "://"), extSvc, "http://"+extSvc) + // do a quick check with short timeout + client := &http.Client{Timeout: 2 * time.Second} + resp, err := client.Get(testURL) + if err == nil { + _ = resp.Body.Close() + } + lastCheckErr = err } + externalServiceCheckResult.Store(extSvc, lastCheckErr) } else { - _ = resp.Body.Close() + lastCheckErr, _ = lastCheckErrAny.(error) } - return val + + if lastCheckErr != nil { + if AllowSkipExternalService() { + t.Skipf("skipping test because %s is not ready", extSvc) + } else { + t.Fatalf("%s is not ready, but skipping is not allowed in CI", extSvc) + } + } + return extSvc } var normalizeHTMLSpacesRegexp = sync.OnceValue(func() (ret struct { diff --git a/services/org/org.go b/services/org/org.go index 953e869426c..6da39eb6dc2 100644 --- a/services/org/org.go +++ b/services/org/org.go @@ -54,14 +54,14 @@ func deleteOrganization(ctx context.Context, org *org_model.Organization) error // DeleteOrganization completely and permanently deletes everything of organization. func DeleteOrganization(ctx context.Context, org *org_model.Organization, purge bool) error { - if err := db.WithTx(ctx, func(ctx context.Context) error { - if purge { - err := repo_service.DeleteOwnerRepositoriesDirectly(ctx, org.AsUser()) - if err != nil { - return err - } + // outside the transaction below, because each repository deletion owns one and deletes storage after committing + if purge { + if err := repo_service.DeleteOwnerRepositoriesDirectly(ctx, org.AsUser()); err != nil { + return err } + } + if err := db.WithTx(ctx, func(ctx context.Context) error { // Check ownership of repository. count, err := repo_model.CountRepositories(ctx, repo_model.CountRepositoryOptions{OwnerID: org.ID}) if err != nil { diff --git a/services/repository/delete.go b/services/repository/delete.go index 38368fa2f4d..cd3f4914460 100644 --- a/services/repository/delete.go +++ b/services/repository/delete.go @@ -5,6 +5,7 @@ package repository import ( "context" + "errors" "fmt" actions_model "gitea.dev/models/actions" @@ -48,9 +49,11 @@ func deleteDBRepository(ctx context.Context, repoID int64) error { return nil } -// DeleteRepository deletes a repository for a user or organization. -// make sure if you call this func to close open sessions (sqlite will otherwise get a deadlock) +// DeleteRepositoryDirectly deletes a repository for a user or organization. func DeleteRepositoryDirectly(ctx context.Context, repoID int64, ignoreOrgTeams ...bool) error { + if db.InTransaction(ctx) { + return errors.New("DeleteRepositoryDirectly must not be called within a transaction, it deletes storage once its own transaction commits") + } ctx, committer, err := db.TxContext(ctx) if err != nil { return err diff --git a/services/repository/delete_test.go b/services/repository/delete_test.go index c747d0087e8..be84b549c18 100644 --- a/services/repository/delete_test.go +++ b/services/repository/delete_test.go @@ -4,6 +4,7 @@ package repository_test import ( + "context" "testing" actions_model "gitea.dev/models/actions" @@ -54,6 +55,16 @@ func TestDeleteOwnerRepositoriesDirectly(t *testing.T) { user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) assert.NoError(t, repo_service.DeleteOwnerRepositoriesDirectly(t.Context(), user)) + + t.Run("RejectedInTransaction", func(t *testing.T) { + unittest.PrepareTestEnv(t) + + err := db.WithTx(t.Context(), func(ctx context.Context) error { + return repo_service.DeleteRepositoryDirectly(ctx, 1) + }) + assert.ErrorContains(t, err, "must not be called within a transaction") + unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}) + }) } func TestDeleteRepositoryDirectlyPurgesRepoScopedRows(t *testing.T) { diff --git a/tests/integration/git_push_test.go b/tests/integration/git_push_test.go index 8f0902c779a..284a07875e5 100644 --- a/tests/integration/git_push_test.go +++ b/tests/integration/git_push_test.go @@ -29,7 +29,7 @@ func TestGitPush(t *testing.T) { func testGitPush(t *testing.T, u *url.URL) { t.Run("Push branches at once", func(t *testing.T) { runTestGitPush(t, u, func(t *testing.T, gitPath string) (pushed, deleted []string) { - for i := range 100 { + for i := range 10 { branchName := fmt.Sprintf("branch-%d", i) pushed = append(pushed, branchName) doGitCreateBranch(gitPath, branchName)(t) @@ -81,7 +81,7 @@ func testGitPush(t *testing.T, u *url.URL) { t.Run("Push branches one by one", func(t *testing.T) { runTestGitPush(t, u, func(t *testing.T, gitPath string) (pushed, deleted []string) { - for i := range 100 { + for i := range 10 { branchName := fmt.Sprintf("branch-%d", i) doGitCreateBranch(gitPath, branchName)(t) doGitPushTestRepository(gitPath, "origin", branchName)(t) @@ -107,14 +107,14 @@ func testGitPush(t *testing.T, u *url.URL) { doGitPushTestRepository(gitPath, "origin", "master")(t) // make sure master is the default branch instead of a branch we are going to delete pushed = append(pushed, "master") - for i := range 100 { + for i := range 10 { branchName := fmt.Sprintf("branch-%d", i) pushed = append(pushed, branchName) doGitCreateBranch(gitPath, branchName)(t) } doGitPushTestRepository(gitPath, "origin", "--all")(t) - for i := range 10 { + for i := range 5 { branchName := fmt.Sprintf("branch-%d", i) doGitPushTestRepository(gitPath, "origin", "--delete", branchName)(t) deleted = append(deleted, branchName) diff --git a/tools/lint-go-all.go b/tools/lint-go-all.go index 0193baccf27..1560bf6798b 100644 --- a/tools/lint-go-all.go +++ b/tools/lint-go-all.go @@ -91,13 +91,21 @@ func main() { _, _ = fmt.Fprintln(os.Stdout, "lint go header ...") succeed := lintGoHeader() + _, _ = fmt.Fprintln(os.Stdout, "lint for linux ...") - succeed = runCmd([]string{"GOOS=linux", "TAGS=bindata"}, "golangci-lint", append([]string{"run", "--build-tags=linux,bindata"}, os.Args[1:]...)) && succeed + lintTagsLinux := "" + if os.Getenv("CI") != "" || strings.Contains(os.Getenv("TAGS"), "bindata") { + // also lint with bindata tag if we are in CI or the "bindata" is explicitly set in the env TAGS + lintTagsLinux = "bindata" + } + succeed = runCmd([]string{"GOOS=linux", "TAGS=" + lintTagsLinux}, "golangci-lint", append([]string{"run", "--build-tags=linux," + lintTagsLinux}, os.Args[1:]...)) && succeed + if os.Getenv("CI") != "" { // only lint for other platforms when in CI, to keep local lint fast _, _ = fmt.Fprintln(os.Stdout, "lint for windows ...") succeed = runCmd([]string{"GOOS=windows", "TAGS=gogit"}, "golangci-lint", append([]string{"run", "--build-tags=windows,gogit"}, os.Args[1:]...)) && succeed } + if !succeed { os.Exit(1) }