diff --git a/.golangci.yml b/.golangci.yml index 839d0eaedb..3cb31b8a3b 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -8,7 +8,17 @@ linters: - gofmt - goimports - govet + - deadcode + - gosimple + - typecheck + - errcheck - bidichk run: timeout: 5m + +issues: + exclude-rules: + - path: woodpecker-go/woodpecker/client.go|server/swagger/swagger.go + linters: + - deadcode diff --git a/agent/runner.go b/agent/runner.go index 7312c3e216..9be11a244a 100644 --- a/agent/runner.go +++ b/agent/runner.go @@ -122,7 +122,9 @@ func (r *Runner) Run(ctx context.Context) error { case <-time.After(time.Minute): logger.Debug().Msg("pipeline lease renewed") - r.client.Extend(ctx, work.ID) + if err := r.client.Extend(ctx, work.ID); err != nil { + log.Error().Err(err).Msg("extending pipeline deadline failed") + } } } }() @@ -159,12 +161,14 @@ func (r *Runner) Run(ctx context.Context) error { loglogger.Debug().Msg("log stream opened") limitedPart := io.LimitReader(part, maxLogsUpload) - logstream := rpc.NewLineWriter(r.client, work.ID, proc.Alias, secrets...) - io.Copy(logstream, limitedPart) + logStream := rpc.NewLineWriter(r.client, work.ID, proc.Alias, secrets...) + if _, err := io.Copy(logStream, limitedPart); err != nil { + log.Error().Err(err).Msg("copy limited logStream part") + } loglogger.Debug().Msg("log stream copied") - data, err := json.Marshal(logstream.Lines()) + data, err := json.Marshal(logStream.Lines()) if err != nil { loglogger.Err(err).Msg("could not marshal logstream") } diff --git a/cli/build/build_list.go b/cli/build/build_list.go index c8fe6af773..4c3f1acc97 100644 --- a/cli/build/build_list.go +++ b/cli/build/build_list.go @@ -78,7 +78,9 @@ func buildList(c *cli.Context) error { if status != "" && build.Status != status { continue } - tmpl.Execute(os.Stdout, build) + if err := tmpl.Execute(os.Stdout, build); err != nil { + return err + } count++ } return nil diff --git a/cli/build/build_queue.go b/cli/build/build_queue.go index 9b212f13fd..184eeeb6fa 100644 --- a/cli/build/build_queue.go +++ b/cli/build/build_queue.go @@ -44,7 +44,9 @@ func buildQueue(c *cli.Context) error { } for _, build := range builds { - tmpl.Execute(os.Stdout, build) + if err := tmpl.Execute(os.Stdout, build); err != nil { + return err + } } return nil } diff --git a/cli/exec/exec.go b/cli/exec/exec.go index 460e526e76..685ab53496 100644 --- a/cli/exec/exec.go +++ b/cli/exec/exec.go @@ -260,8 +260,7 @@ var defaultLogger = pipeline.LogFunc(func(proc *backend.Step, rc multipart.Reade return err } - logstream := NewLineWriter(proc.Alias) - io.Copy(logstream, part) - - return nil + logStream := NewLineWriter(proc.Alias) + _, err = io.Copy(logStream, part) + return err }) diff --git a/cli/lint/lint.go b/cli/lint/lint.go index c61805e46f..45d9978da6 100644 --- a/cli/lint/lint.go +++ b/cli/lint/lint.go @@ -43,7 +43,7 @@ func lint(c *cli.Context) error { // check if it is a regular file (not dir) if info.Mode().IsRegular() && strings.HasSuffix(info.Name(), ".yml") { fmt.Println("#", info.Name()) - lintFile(path) + _ = lintFile(path) // TODO: should we drop errors or store them and report back? fmt.Println("") return nil } diff --git a/cli/registry/registry_list.go b/cli/registry/registry_list.go index 6b216f8c21..bd80874451 100644 --- a/cli/registry/registry_list.go +++ b/cli/registry/registry_list.go @@ -49,7 +49,9 @@ func registryList(c *cli.Context) error { return err } for _, registry := range list { - tmpl.Execute(os.Stdout, registry) + if err := tmpl.Execute(os.Stdout, registry); err != nil { + return err + } } return nil } diff --git a/cli/repo/repo_list.go b/cli/repo/repo_list.go index 7335e1ed4a..c319e10757 100644 --- a/cli/repo/repo_list.go +++ b/cli/repo/repo_list.go @@ -45,7 +45,9 @@ func repoList(c *cli.Context) error { if org != "" && org != repo.Owner { continue } - tmpl.Execute(os.Stdout, repo) + if err := tmpl.Execute(os.Stdout, repo); err != nil { + return err + } } return nil } diff --git a/cli/repo/repo_sync.go b/cli/repo/repo_sync.go index bbf373b56b..22e4fa7c70 100644 --- a/cli/repo/repo_sync.go +++ b/cli/repo/repo_sync.go @@ -41,7 +41,9 @@ func repoSync(c *cli.Context) error { if org != "" && org != repo.Owner { continue } - tmpl.Execute(os.Stdout, repo) + if err := tmpl.Execute(os.Stdout, repo); err != nil { + return err + } } return nil } diff --git a/cli/secret/secret_list.go b/cli/secret/secret_list.go index b5a087742f..fabd8e7f3a 100644 --- a/cli/secret/secret_list.go +++ b/cli/secret/secret_list.go @@ -50,7 +50,9 @@ func secretList(c *cli.Context) error { return err } for _, registry := range list { - tmpl.Execute(os.Stdout, registry) + if err := tmpl.Execute(os.Stdout, registry); err != nil { + return err + } } return nil } diff --git a/cli/user/user_list.go b/cli/user/user_list.go index 8783d37d82..a0891d7417 100644 --- a/cli/user/user_list.go +++ b/cli/user/user_list.go @@ -36,7 +36,9 @@ func userList(c *cli.Context) error { return err } for _, user := range users { - tmpl.Execute(os.Stdout, user) + if err := tmpl.Execute(os.Stdout, user); err != nil { + return err + } } return nil } diff --git a/cmd/agent/health.go b/cmd/agent/health.go index 9c00bb4a4d..3645509eff 100644 --- a/cmd/agent/health.go +++ b/cmd/agent/health.go @@ -19,6 +19,7 @@ import ( "fmt" "net/http" + "github.com/rs/zerolog/log" "github.com/urfave/cli/v2" "github.com/woodpecker-ci/woodpecker/agent" @@ -46,7 +47,7 @@ func handleHeartbeat(w http.ResponseWriter, r *http.Request) { func handleVersion(w http.ResponseWriter, r *http.Request) { w.WriteHeader(200) w.Header().Add("Content-Type", "text/json") - json.NewEncoder(w).Encode(versionResp{ + _ = json.NewEncoder(w).Encode(versionResp{ Source: "https://github.com/woodpecker-ci/woodpecker", Version: version.String(), }) @@ -59,7 +60,9 @@ func handleStats(w http.ResponseWriter, r *http.Request) { w.WriteHeader(500) } w.Header().Add("Content-Type", "text/json") - counter.WriteTo(w) + if _, err := counter.WriteTo(w); err != nil { + log.Error().Err(err).Msg("handleStats") + } } type versionResp struct { diff --git a/cmd/agent/signal.go b/cmd/agent/signal.go index 41782d7d36..e68917d6d6 100644 --- a/cmd/agent/signal.go +++ b/cmd/agent/signal.go @@ -12,14 +12,6 @@ import ( "syscall" ) -// WithContext returns a copy of parent context whose Done channel is closed -// when an os interrupt signal is received. -func WithContext(ctx context.Context) context.Context { - return WithContextFunc(ctx, func() { - println("interrupt received, terminating process") - }) -} - // WithContextFunc returns a copy of parent context that is cancelled when // an os interrupt signal is received. The callback function f is invoked // before cancellation. diff --git a/cmd/server/main.go b/cmd/server/main.go index 53871eedaa..58107a0d1c 100644 --- a/cmd/server/main.go +++ b/cmd/server/main.go @@ -19,19 +19,21 @@ import ( "os" "github.com/joho/godotenv" - _ "github.com/joho/godotenv/autoload" + "github.com/rs/zerolog/log" "github.com/urfave/cli/v2" "github.com/woodpecker-ci/woodpecker/version" ) func main() { - godotenv.Load(".env") + if err := godotenv.Load(".env"); err != nil { + log.Error().Err(err).Msg("load godotenv failed") + } app := cli.NewApp() app.Name = "woodpecker-server" app.Version = version.String() app.Usage = "woodpecker server" - app.Action = loop + app.Action = run app.Flags = flags app.Before = before diff --git a/cmd/server/server.go b/cmd/server/server.go index b93bce85b6..7d1b16344a 100644 --- a/cmd/server/server.go +++ b/cmd/server/server.go @@ -49,7 +49,7 @@ import ( "github.com/woodpecker-ci/woodpecker/server/store" ) -func loop(c *cli.Context) error { +func run(c *cli.Context) error { if c.Bool("pretty") { log.Logger = log.Output( @@ -224,7 +224,9 @@ func loop(c *cli.Context) error { } dir := cacheDir() - os.MkdirAll(dir, 0700) + if err := os.MkdirAll(dir, 0700); err != nil { + return err + } manager := &autocert.Manager{ Prompt: autocert.AcceptTOS, @@ -259,7 +261,9 @@ func setupEvilGlobals(c *cli.Context, v store.Store, r remote.Remote) { server.Config.Services.Queue = setupQueue(c, v) server.Config.Services.Logs = logging.New() server.Config.Services.Pubsub = pubsub.New() - server.Config.Services.Pubsub.Create(context.Background(), "topic/events") + if err := server.Config.Services.Pubsub.Create(context.Background(), "topic/events"); err != nil { + log.Error().Err(err).Msg("could not create pubsub service") + } server.Config.Services.Registries = setupRegistryService(c, v) server.Config.Services.Secrets = setupSecretService(c, v) server.Config.Services.Senders = sender.New(v, v) diff --git a/go.mod b/go.mod index bb3994ee60..e8a8b60a71 100644 --- a/go.mod +++ b/go.mod @@ -35,6 +35,7 @@ require ( github.com/moby/term v0.0.0-20210619224110-3f7ff695adc6 github.com/morikuni/aec v1.0.0 // indirect github.com/mrjones/oauth v0.0.0-20190623134757-126b35219450 + github.com/pkg/errors v0.9.1 github.com/prometheus/client_golang v1.11.0 github.com/rs/zerolog v1.25.0 github.com/stretchr/objx v0.3.0 // indirect diff --git a/pipeline/backend/docker/convert.go b/pipeline/backend/docker/convert.go index 4a73cbbe49..8cfe93b17d 100644 --- a/pipeline/backend/docker/convert.go +++ b/pipeline/backend/docker/convert.go @@ -79,7 +79,7 @@ func toHostConfig(proc *backend.Step) *container.HostConfig { } config.Tmpfs = map[string]string{} for _, path := range proc.Tmpfs { - if strings.Index(path, ":") == -1 { + if !strings.Contains(path, ":") { config.Tmpfs[path] = "" continue } @@ -89,9 +89,6 @@ func toHostConfig(proc *backend.Step) *container.HostConfig { } config.Tmpfs[parts[0]] = parts[1] } - // if proc.OomKillDisable { - // config.OomKillDisable = &proc.OomKillDisable - // } return config } diff --git a/pipeline/backend/docker/docker.go b/pipeline/backend/docker/docker.go index 5425736eac..aa9fe7a09e 100644 --- a/pipeline/backend/docker/docker.go +++ b/pipeline/backend/docker/docker.go @@ -12,6 +12,7 @@ import ( "github.com/moby/moby/pkg/jsonmessage" "github.com/moby/moby/pkg/stdcopy" "github.com/moby/term" + "github.com/rs/zerolog/log" "github.com/woodpecker-ci/woodpecker/pipeline/backend" ) @@ -80,7 +81,9 @@ func (e *engine) Exec(ctx context.Context, proc *backend.Step) error { defer responseBody.Close() fd, isTerminal := term.GetFdInfo(os.Stdout) - jsonmessage.DisplayJSONMessagesStream(responseBody, os.Stdout, fd, isTerminal, nil) + if err := jsonmessage.DisplayJSONMessagesStream(responseBody, os.Stdout, fd, isTerminal, nil); err != nil { + log.Error().Err(err).Msg("DisplayJSONMessagesStream") + } } // fix for drone/drone#1917 if perr != nil && proc.AuthConfig.Password != "" { @@ -98,7 +101,9 @@ func (e *engine) Exec(ctx context.Context, proc *backend.Step) error { } defer responseBody.Close() fd, isTerminal := term.GetFdInfo(os.Stdout) - jsonmessage.DisplayJSONMessagesStream(responseBody, os.Stdout, fd, isTerminal, nil) + if err := jsonmessage.DisplayJSONMessagesStream(responseBody, os.Stdout, fd, isTerminal, nil); err != nil { + log.Error().Err(err).Msg("DisplayJSONMessagesStream") + } _, err = e.client.ContainerCreate(ctx, config, hostConfig, nil, nil, proc.Name) } @@ -162,11 +167,12 @@ func (e *engine) Tail(ctx context.Context, proc *backend.Step) (io.ReadCloser, e } rc, wc := io.Pipe() + // de multiplex 'logs' who contains two streams, previously multiplexed together using StdWriter go func() { - stdcopy.StdCopy(wc, wc, logs) - logs.Close() - wc.Close() - rc.Close() + _, _ = stdcopy.StdCopy(wc, wc, logs) + _ = logs.Close() + _ = wc.Close() + _ = rc.Close() }() return rc, nil } @@ -174,15 +180,23 @@ func (e *engine) Tail(ctx context.Context, proc *backend.Step) (io.ReadCloser, e func (e *engine) Destroy(_ context.Context, conf *backend.Config) error { for _, stage := range conf.Stages { for _, step := range stage.Steps { - e.client.ContainerKill(noContext, step.Name, "9") - e.client.ContainerRemove(noContext, step.Name, removeOpts) + if err := e.client.ContainerKill(noContext, step.Name, "9"); err != nil { + log.Error().Err(err).Msgf("could not kill container '%s'", stage.Name) + } + if err := e.client.ContainerRemove(noContext, step.Name, removeOpts); err != nil { + log.Error().Err(err).Msgf("could not remove container '%s'", stage.Name) + } } } for _, v := range conf.Volumes { - e.client.VolumeRemove(noContext, v.Name, true) + if err := e.client.VolumeRemove(noContext, v.Name, true); err != nil { + log.Error().Err(err).Msgf("could not remove volume '%s'", v.Name) + } } for _, n := range conf.Networks { - e.client.NetworkRemove(noContext, n.Name) + if err := e.client.NetworkRemove(noContext, n.Name); err != nil { + log.Error().Err(err).Msgf("could not remove network '%s'", n.Name) + } } return nil } diff --git a/pipeline/frontend/metadata.go b/pipeline/frontend/metadata.go index d7a320d3b7..17a5179c65 100644 --- a/pipeline/frontend/metadata.go +++ b/pipeline/frontend/metadata.go @@ -233,7 +233,7 @@ func (m *Metadata) EnvironDrone() map[string]string { return params } -var pullRegexp = regexp.MustCompile("\\d+") +var pullRegexp = regexp.MustCompile(`\d+`) func (m *Metadata) SetPlatform(platform string) { if platform == "" { diff --git a/pipeline/frontend/yaml/compiler/convert.go b/pipeline/frontend/yaml/compiler/convert.go index 592a72af7b..9832ca4aa8 100644 --- a/pipeline/frontend/yaml/compiler/convert.go +++ b/pipeline/frontend/yaml/compiler/convert.go @@ -5,6 +5,8 @@ import ( "path" "strings" + "github.com/rs/zerolog/log" + "github.com/woodpecker-ci/woodpecker/pipeline/backend" "github.com/woodpecker-ci/woodpecker/pipeline/frontend/yaml" ) @@ -67,12 +69,14 @@ func (c *Compiler) createProcess(name string, container *yaml.Container, section detached = true } - if detached == false || len(container.Commands) != 0 { + if !detached || len(container.Commands) != 0 { workingdir = path.Join(c.base, c.path) } - if detached == false { - paramsToEnv(container.Vargs, environment) + if !detached { + if err := paramsToEnv(container.Vargs, environment); err != nil { + log.Error().Err(err).Msg("paramsToEnv") + } } if len(container.Commands) != 0 { diff --git a/pipeline/frontend/yaml/compiler/params_test.go b/pipeline/frontend/yaml/compiler/params_test.go index 66d87dc23d..017c5fbf65 100644 --- a/pipeline/frontend/yaml/compiler/params_test.go +++ b/pipeline/frontend/yaml/compiler/params_test.go @@ -5,6 +5,7 @@ import ( "testing" "github.com/kr/pretty" + "github.com/stretchr/testify/assert" ) func TestParamsToEnv(t *testing.T) { @@ -28,7 +29,7 @@ func TestParamsToEnv(t *testing.T) { "PLUGIN_COMPLEX": `[{"name":"Jack"},{"name":"Jill"}]`, } got := map[string]string{} - paramsToEnv(from, got) + assert.NoError(t, paramsToEnv(from, got)) if !reflect.DeepEqual(want, got) { t.Errorf("Problem converting plugin parameters to environment variables") diff --git a/pipeline/frontend/yaml/constraint.go b/pipeline/frontend/yaml/constraint.go index a605a6dd90..daf67e26ee 100644 --- a/pipeline/frontend/yaml/constraint.go +++ b/pipeline/frontend/yaml/constraint.go @@ -164,8 +164,8 @@ func (c *ConstraintMap) UnmarshalYAML(unmarshal func(interface{}) error) error { out2 := map[string]string{} - unmarshal(&out1) - unmarshal(&out2) + _ = unmarshal(&out1) // it contains include and exclude statement + _ = unmarshal(&out2) // it contains no include/exclude statement, assume include as default c.Include = out1.Include c.Exclude = out1.Exclude diff --git a/pipeline/frontend/yaml/constraint_test.go b/pipeline/frontend/yaml/constraint_test.go index 09e97b76bd..40373c42f9 100644 --- a/pipeline/frontend/yaml/constraint_test.go +++ b/pipeline/frontend/yaml/constraint_test.go @@ -3,6 +3,7 @@ package yaml import ( "testing" + "github.com/stretchr/testify/assert" "gopkg.in/yaml.v3" "github.com/woodpecker-ci/woodpecker/pipeline/frontend" @@ -142,7 +143,7 @@ func TestConstraint(t *testing.T) { }, } for _, test := range testdata { - c := parseConstraint(test.conf) + c := parseConstraint(t, test.conf) got, want := c.Match(test.with), test.want if got != want { t.Errorf("Expect %q matches %q is %v", test.with, test.conf, want) @@ -250,7 +251,7 @@ func TestConstraintList(t *testing.T) { }, } for _, test := range testdata { - c := parseConstraintPath(test.conf) + c := parseConstraintPath(t, test.conf) got, want := c.Match(test.with, test.message), test.want if got != want { t.Errorf("Expect %q matches %q should be %v got %v", test.with, test.conf, want, got) @@ -366,7 +367,7 @@ func TestConstraintMap(t *testing.T) { }, } for _, test := range testdata { - c := parseConstraintMap(test.conf) + c := parseConstraintMap(t, test.conf) got, want := c.Match(test.with), test.want if got != want { t.Errorf("Expect %q matches %q is %v", test.with, test.conf, want) @@ -454,7 +455,7 @@ func TestConstraints(t *testing.T) { }, } for _, test := range testdata { - c := parseConstraints(test.conf) + c := parseConstraints(t, test.conf) got, want := c.Match(test.with), test.want if got != want { t.Errorf("Expect %+v matches %q is %v", test.with, test.conf, want) @@ -462,26 +463,26 @@ func TestConstraints(t *testing.T) { } } -func parseConstraints(s string) *Constraints { +func parseConstraints(t *testing.T, s string) *Constraints { c := &Constraints{} - yaml.Unmarshal([]byte(s), c) + assert.NoError(t, yaml.Unmarshal([]byte(s), c)) return c } -func parseConstraint(s string) *Constraint { +func parseConstraint(t *testing.T, s string) *Constraint { c := &Constraint{} - yaml.Unmarshal([]byte(s), c) + assert.NoError(t, yaml.Unmarshal([]byte(s), c)) return c } -func parseConstraintMap(s string) *ConstraintMap { +func parseConstraintMap(t *testing.T, s string) *ConstraintMap { c := &ConstraintMap{} - yaml.Unmarshal([]byte(s), c) + assert.NoError(t, yaml.Unmarshal([]byte(s), c)) return c } -func parseConstraintPath(s string) *ConstraintPath { +func parseConstraintPath(t *testing.T, s string) *ConstraintPath { c := &ConstraintPath{} - yaml.Unmarshal([]byte(s), c) + assert.NoError(t, yaml.Unmarshal([]byte(s), c)) return c } diff --git a/pipeline/frontend/yaml/linter/linter.go b/pipeline/frontend/yaml/linter/linter.go index da0dab3782..c4bf621161 100644 --- a/pipeline/frontend/yaml/linter/linter.go +++ b/pipeline/frontend/yaml/linter/linter.go @@ -48,7 +48,7 @@ func (l *Linter) lint(containers []*yaml.Container, block uint8) error { if err := l.lintImage(container); err != nil { return err } - if l.trusted == false { + if !l.trusted { if err := l.lintTrusted(container); err != nil { return err } diff --git a/pipeline/frontend/yaml/types/bool.go b/pipeline/frontend/yaml/types/bool.go index 20d77f65d8..a496fc6d31 100644 --- a/pipeline/frontend/yaml/types/bool.go +++ b/pipeline/frontend/yaml/types/bool.go @@ -14,7 +14,9 @@ type BoolTrue struct { // UnmarshalYAML implements custom Yaml unmarshaling. func (b *BoolTrue) UnmarshalYAML(value *yaml.Node) error { var s string - value.Decode(&s) + if err := value.Decode(&s); err != nil { + return err + } v, err := strconv.ParseBool(s) if err == nil { diff --git a/pipeline/frontend/yaml/types/types_yaml_test.go b/pipeline/frontend/yaml/types/types_yaml_test.go index f56735d8cd..87869a25a0 100644 --- a/pipeline/frontend/yaml/types/types_yaml_test.go +++ b/pipeline/frontend/yaml/types/types_yaml_test.go @@ -16,7 +16,7 @@ type StructStringorInt struct { func TestStringorIntYaml(t *testing.T) { for _, str := range []string{`{foo: 10}`, `{foo: "10"}`} { s := StructStringorInt{} - yaml.Unmarshal([]byte(str), &s) + assert.NoError(t, yaml.Unmarshal([]byte(str), &s)) assert.Equal(t, StringorInt(10), s.Foo) @@ -24,7 +24,7 @@ func TestStringorIntYaml(t *testing.T) { assert.Nil(t, err) s2 := StructStringorInt{} - yaml.Unmarshal(d, &s2) + assert.NoError(t, yaml.Unmarshal(d, &s2)) assert.Equal(t, StringorInt(10), s2.Foo) } @@ -38,7 +38,7 @@ func TestStringorsliceYaml(t *testing.T) { str := `{foo: [bar, baz]}` s := StructStringorslice{} - yaml.Unmarshal([]byte(str), &s) + assert.NoError(t, yaml.Unmarshal([]byte(str), &s)) assert.Equal(t, Stringorslice{"bar", "baz"}, s.Foo) @@ -46,7 +46,7 @@ func TestStringorsliceYaml(t *testing.T) { assert.Nil(t, err) s2 := StructStringorslice{} - yaml.Unmarshal(d, &s2) + assert.NoError(t, yaml.Unmarshal(d, &s2)) assert.Equal(t, Stringorslice{"bar", "baz"}, s2.Foo) } @@ -60,7 +60,7 @@ func TestSliceOrMapYaml(t *testing.T) { str := `{foos: [bar=baz, far=faz]}` s := StructSliceorMap{} - yaml.Unmarshal([]byte(str), &s) + assert.NoError(t, yaml.Unmarshal([]byte(str), &s)) assert.Equal(t, SliceorMap{"bar": "baz", "far": "faz"}, s.Foos) @@ -68,7 +68,7 @@ func TestSliceOrMapYaml(t *testing.T) { assert.Nil(t, err) s2 := StructSliceorMap{} - yaml.Unmarshal(d, &s2) + assert.NoError(t, yaml.Unmarshal(d, &s2)) assert.Equal(t, SliceorMap{"bar": "baz", "far": "faz"}, s2.Foos) } @@ -95,7 +95,7 @@ func TestStr2SliceOrMapPtrMap(t *testing.T) { assert.Nil(t, err) s2 := map[string]*StructSliceorMap{} - yaml.Unmarshal(d, &s2) + assert.NoError(t, yaml.Unmarshal(d, &s2)) assert.Equal(t, s, s2) } diff --git a/pipeline/pipeline.go b/pipeline/pipeline.go index 11c805e1af..2b1e5dadbc 100644 --- a/pipeline/pipeline.go +++ b/pipeline/pipeline.go @@ -4,6 +4,7 @@ import ( "context" "time" + "github.com/rs/zerolog/log" "golang.org/x/sync/errgroup" "github.com/woodpecker-ci/woodpecker/pipeline/backend" @@ -55,7 +56,9 @@ func New(spec *backend.Config, opts ...Option) *Runtime { // Run starts the runtime and waits for it to complete. func (r *Runtime) Run() error { defer func() { - r.engine.Destroy(r.ctx, r.spec) + if err := r.engine.Destroy(r.ctx, r.spec); err != nil { + log.Error().Err(err).Msg("could not destroy engine") + } }() r.started = time.Now().Unix() @@ -105,9 +108,9 @@ func (r *Runtime) execAll(procs []*backend.Step) <-chan error { func (r *Runtime) exec(proc *backend.Step) error { switch { - case r.err != nil && proc.OnFailure == false: + case r.err != nil && !proc.OnFailure: return nil - case r.err == nil && proc.OnSuccess == false: + case r.err == nil && !proc.OnSuccess: return nil } @@ -135,8 +138,10 @@ func (r *Runtime) exec(proc *backend.Step) error { } go func() { - r.logger.Log(proc, multipart.New(rc)) - rc.Close() + if err := r.logger.Log(proc, multipart.New(rc)); err != nil { + log.Error().Err(err).Msg("process logging failed") + } + _ = rc.Close() }() } diff --git a/pipeline/rpc/client_grpc.go b/pipeline/rpc/client_grpc.go index a9440d794a..39d359f89b 100644 --- a/pipeline/rpc/client_grpc.go +++ b/pipeline/rpc/client_grpc.go @@ -73,7 +73,9 @@ func (c *client) Next(ctx context.Context, f Filter) (*Pipeline, error) { p.ID = res.GetPipeline().GetId() p.Timeout = res.GetPipeline().GetTimeout() p.Config = new(backend.Config) - json.Unmarshal(res.GetPipeline().GetPayload(), p.Config) + if err := json.Unmarshal(res.GetPipeline().GetPayload(), p.Config); err != nil { + log.Error().Err(err).Msgf("could not unmarshal pipeline config of '%s'", p.ID) + } return p, nil } diff --git a/pipeline/rpc/line.go b/pipeline/rpc/line.go index 1978dcb4ba..e6ef57e933 100644 --- a/pipeline/rpc/line.go +++ b/pipeline/rpc/line.go @@ -5,6 +5,8 @@ import ( "fmt" "strings" "time" + + "github.com/rs/zerolog/log" ) // Identifies the type of line in the logs. @@ -78,7 +80,9 @@ func (w *LineWriter) Write(p []byte) (n int, err error) { Time: int64(time.Since(w.now).Seconds()), Type: LineStdout, } - w.peer.Log(context.Background(), w.id, line) + if err := w.peer.Log(context.Background(), w.id, line); err != nil { + log.Error().Err(err).Msgf("fail to write pipeline log to peer '%s'", w.id) + } w.num++ // for _, part := range bytes.Split(p, []byte{'\n'}) { diff --git a/server/api/build.go b/server/api/build.go index 7928d53a35..96c8bc5363 100644 --- a/server/api/build.go +++ b/server/api/build.go @@ -19,7 +19,6 @@ package api import ( "bytes" - "context" "fmt" "io" "net/http" @@ -42,7 +41,7 @@ func GetBuilds(c *gin.Context) { repo := session.Repo(c) page, err := strconv.Atoi(c.DefaultQuery("page", "1")) if err != nil { - c.AbortWithError(http.StatusBadRequest, err) + _ = c.AbortWithError(http.StatusBadRequest, err) return } @@ -64,13 +63,13 @@ func GetBuild(c *gin.Context) { repo := session.Repo(c) num, err := strconv.ParseInt(c.Param("number"), 10, 64) if err != nil { - c.AbortWithError(http.StatusBadRequest, err) + _ = c.AbortWithError(http.StatusBadRequest, err) return } build, err := store_.GetBuildNumber(repo, num) if err != nil { - c.AbortWithError(http.StatusInternalServerError, err) + _ = c.AbortWithError(http.StatusInternalServerError, err) return } files, _ := store_.FileList(build) @@ -109,26 +108,28 @@ func GetBuildLogs(c *gin.Context) { build, err := store_.GetBuildNumber(repo, num) if err != nil { - c.AbortWithError(404, err) + _ = c.AbortWithError(404, err) return } proc, err := store_.ProcChild(build, ppid, name) if err != nil { - c.AbortWithError(404, err) + _ = c.AbortWithError(404, err) return } rc, err := store_.LogFind(proc) if err != nil { - c.AbortWithError(404, err) + _ = c.AbortWithError(404, err) return } defer rc.Close() c.Header("Content-Type", "application/json") - io.Copy(c.Writer, rc) + if _, err := io.Copy(c.Writer, rc); err != nil { + log.Error().Err(err).Msg("could not copy log to http response") + } } func GetProcLogs(c *gin.Context) { @@ -142,26 +143,28 @@ func GetProcLogs(c *gin.Context) { build, err := store_.GetBuildNumber(repo, num) if err != nil { - c.AbortWithError(404, err) + _ = c.AbortWithError(404, err) return } proc, err := store_.ProcFind(build, pid) if err != nil { - c.AbortWithError(404, err) + _ = c.AbortWithError(404, err) return } rc, err := store_.LogFind(proc) if err != nil { - c.AbortWithError(404, err) + _ = c.AbortWithError(404, err) return } defer rc.Close() c.Header("Content-Type", "application/json") - io.Copy(c.Writer, rc) + if _, err := io.Copy(c.Writer, rc); err != nil { + log.Error().Err(err).Msg("could not copy log to http response") + } } // DeleteBuild cancels a build @@ -203,9 +206,19 @@ func DeleteBuild(c *gin.Context) { procToEvict = append(procToEvict, fmt.Sprint(proc.ID)) } } - server.Config.Services.Queue.EvictAtOnce(context.Background(), procToEvict) - server.Config.Services.Queue.ErrorAtOnce(context.Background(), procToEvict, queue.ErrCancel) - server.Config.Services.Queue.ErrorAtOnce(context.Background(), procToCancel, queue.ErrCancel) + + if err := server.Config.Services.Queue.EvictAtOnce(c, procToEvict); err != nil { + _ = c.AbortWithError(http.StatusInternalServerError, err) + return + } + if err := server.Config.Services.Queue.ErrorAtOnce(c, procToEvict, queue.ErrCancel); err != nil { + _ = c.AbortWithError(http.StatusInternalServerError, err) + return + } + if err := server.Config.Services.Queue.ErrorAtOnce(c, procToCancel, queue.ErrCancel); err != nil { + _ = c.AbortWithError(http.StatusInternalServerError, err) + return + } // Then update the DB status for pending builds // Running ones will be set when the agents stop on the cancel signal @@ -225,7 +238,7 @@ func DeleteBuild(c *gin.Context) { killedBuild, err := shared.UpdateToStatusKilled(store_, *build) if err != nil { - c.AbortWithError(500, err) + _ = c.AbortWithError(500, err) return } @@ -234,11 +247,13 @@ func DeleteBuild(c *gin.Context) { if build.Status == model.StatusPending { procs, err = store_.ProcList(killedBuild) if err != nil { - c.AbortWithError(404, err) + _ = c.AbortWithError(404, err) return } killedBuild.Procs = model.Tree(procs) - publishToTopic(c, killedBuild, repo, model.Cancelled) + if err := publishToTopic(c, killedBuild, repo, model.Cancelled); err != nil { + log.Error().Err(err).Msg("publishToTopic") + } } c.String(204, "") @@ -255,7 +270,7 @@ func PostApproval(c *gin.Context) { build, err := store_.GetBuildNumber(repo, num) if err != nil { - c.AbortWithError(404, err) + _ = c.AbortWithError(404, err) return } if build.Status != model.StatusBlocked { @@ -267,7 +282,7 @@ func PostApproval(c *gin.Context) { configs, err := server.Config.Storage.Config.ConfigsForBuild(build.ID) if err != nil { log.Error().Msgf("failure to get build config for %s. %s", repo.FullName, err) - c.AbortWithError(404, err) + _ = c.AbortWithError(404, err) return } @@ -347,8 +362,12 @@ func PostApproval(c *gin.Context) { } }() - publishToTopic(c, build, repo, model.Enqueued) - queueBuild(build, repo, buildItems) + if err := publishToTopic(c, build, repo, model.Enqueued); err != nil { + log.Error().Err(err).Msg("publishToTopic") + } + if err := queueBuild(build, repo, buildItems); err != nil { + log.Error().Err(err).Msg("queueBuild") + } } func PostDecline(c *gin.Context) { @@ -363,7 +382,7 @@ func PostDecline(c *gin.Context) { build, err := store_.GetBuildNumber(repo, num) if err != nil { - c.AbortWithError(404, err) + _ = c.AbortWithError(404, err) return } if build.Status != model.StatusBlocked { @@ -402,21 +421,21 @@ func PostBuild(c *gin.Context) { num, err := strconv.ParseInt(c.Param("number"), 10, 64) if err != nil { - c.AbortWithError(http.StatusBadRequest, err) + _ = c.AbortWithError(http.StatusBadRequest, err) return } user, err := store_.GetUser(repo.UserID) if err != nil { log.Error().Msgf("failure to find repo owner %s. %s", repo.FullName, err) - c.AbortWithError(500, err) + _ = c.AbortWithError(500, err) return } build, err := store_.GetBuildNumber(repo, num) if err != nil { log.Error().Msgf("failure to get build %d. %s", num, err) - c.AbortWithError(404, err) + _ = c.AbortWithError(404, err) return } @@ -431,9 +450,13 @@ func PostBuild(c *gin.Context) { // may be stale. Therefore, we should refresh prior to dispatching // the job. if refresher, ok := remote_.(remote.Refresher); ok { - ok, _ := refresher.Refresh(c, user) - if ok { - store_.UpdateUser(user) + ok, err := refresher.Refresh(c, user) + if err != nil { + log.Error().Err(err).Msgf("refresh oauth token of user '%s' failed", user.Login) + } else if ok { + if err := store_.UpdateUser(user); err != nil { + log.Error().Err(err).Msg("fail to save user to store after refresh oauth token") + } } } @@ -441,14 +464,14 @@ func PostBuild(c *gin.Context) { configs, err := server.Config.Storage.Config.ConfigsForBuild(build.ID) if err != nil { log.Error().Msgf("failure to get build config for %s. %s", repo.FullName, err) - c.AbortWithError(404, err) + _ = c.AbortWithError(404, err) return } netrc, err := remote_.Netrc(user, repo) if err != nil { log.Error().Msgf("failure to generate netrc for %s. %s", repo.FullName, err) - c.AbortWithError(500, err) + _ = c.AbortWithError(500, err) return } @@ -479,7 +502,7 @@ func PostBuild(c *gin.Context) { err = persistBuildConfigs(configs, build.ID) if err != nil { log.Error().Msgf("failure to persist build config for %s. %s", repo.FullName, err) - c.AbortWithError(500, err) + _ = c.AbortWithError(500, err) return } @@ -552,8 +575,12 @@ func PostBuild(c *gin.Context) { } c.JSON(202, build) - publishToTopic(c, build, repo, model.Enqueued) - queueBuild(build, repo, buildItems) + if err := publishToTopic(c, build, repo, model.Enqueued); err != nil { + log.Error().Err(err).Msg("publishToTopic") + } + if err := queueBuild(build, repo, buildItems); err != nil { + log.Error().Err(err).Msg("queueBuild") + } } func DeleteBuildLogs(c *gin.Context) { @@ -565,13 +592,13 @@ func DeleteBuildLogs(c *gin.Context) { build, err := store_.GetBuildNumber(repo, num) if err != nil { - c.AbortWithError(404, err) + _ = c.AbortWithError(404, err) return } procs, err := store_.ProcList(build) if err != nil { - c.AbortWithError(404, err) + _ = c.AbortWithError(404, err) return } diff --git a/server/api/file.go b/server/api/file.go index 51bb9503bf..6232cbc72d 100644 --- a/server/api/file.go +++ b/server/api/file.go @@ -21,6 +21,7 @@ import ( "strings" "github.com/gin-gonic/gin" + "github.com/rs/zerolog/log" "github.com/woodpecker-ci/woodpecker/server/router/middleware/session" "github.com/woodpecker-ci/woodpecker/server/store" @@ -31,20 +32,20 @@ func FileList(c *gin.Context) { store_ := store.FromContext(c) num, err := strconv.ParseInt(c.Param("number"), 10, 64) if err != nil { - c.AbortWithError(http.StatusBadRequest, err) + _ = c.AbortWithError(http.StatusBadRequest, err) return } repo := session.Repo(c) build, err := store_.GetBuildNumber(repo, num) if err != nil { - c.AbortWithError(http.StatusInternalServerError, err) + _ = c.AbortWithError(http.StatusInternalServerError, err) return } files, err := store_.FileList(build) if err != nil { - c.AbortWithError(http.StatusInternalServerError, err) + _ = c.AbortWithError(http.StatusInternalServerError, err) return } @@ -65,25 +66,25 @@ func FileGet(c *gin.Context) { num, err := strconv.ParseInt(c.Param("number"), 10, 64) if err != nil { - c.AbortWithError(http.StatusBadRequest, err) + _ = c.AbortWithError(http.StatusBadRequest, err) return } pid, err := strconv.Atoi(c.Param("proc")) if err != nil { - c.AbortWithError(http.StatusBadRequest, err) + _ = c.AbortWithError(http.StatusBadRequest, err) return } build, err := store_.GetBuildNumber(repo, num) if err != nil { - c.AbortWithError(http.StatusInternalServerError, err) + _ = c.AbortWithError(http.StatusInternalServerError, err) return } proc, err := store_.ProcFind(build, pid) if err != nil { - c.AbortWithError(http.StatusInternalServerError, err) + _ = c.AbortWithError(http.StatusInternalServerError, err) return } @@ -110,5 +111,7 @@ func FileGet(c *gin.Context) { c.Header("Content-Type", "application/json") } - io.Copy(c.Writer, rc) + if _, err := io.Copy(c.Writer, rc); err != nil { + log.Error().Err(err).Msg("could not copy file to http response") + } } diff --git a/server/api/hook.go b/server/api/hook.go index 1e6de75a3b..22b7157792 100644 --- a/server/api/hook.go +++ b/server/api/hook.go @@ -82,7 +82,7 @@ func PostHook(c *gin.Context) { tmpRepo, build, err := remote_.Hook(c.Request) if err != nil { log.Error().Msgf("failure to parse hook. %s", err) - c.AbortWithError(400, err) + _ = c.AbortWithError(400, err) return } if build == nil { @@ -107,12 +107,12 @@ func PostHook(c *gin.Context) { repo, err := store_.GetRepoName(tmpRepo.Owner + "/" + tmpRepo.Name) if err != nil { log.Error().Msgf("failure to find repo %s/%s from hook. %s", tmpRepo.Owner, tmpRepo.Name, err) - c.AbortWithError(404, err) + _ = c.AbortWithError(404, err) return } if !repo.IsActive { log.Error().Msgf("ignoring hook. %s/%s is inactive.", tmpRepo.Owner, tmpRepo.Name) - c.AbortWithError(204, err) + _ = c.AbortWithError(204, err) return } @@ -122,7 +122,7 @@ func PostHook(c *gin.Context) { }) if err != nil { log.Error().Msgf("failure to parse token from hook for %s. %s", repo.FullName, err) - c.AbortWithError(400, err) + _ = c.AbortWithError(400, err) return } if parsed.Text != repo.FullName { @@ -139,7 +139,7 @@ func PostHook(c *gin.Context) { if build.Event == model.EventPull && !repo.AllowPull { log.Info().Msgf("ignoring hook. repo %s is disabled for pull requests.", repo.FullName) - c.Writer.Write([]byte("pulls are disabled on woodpecker for this repo")) + _, _ = c.Writer.Write([]byte("pulls are disabled on woodpecker for this repo")) c.Writer.WriteHeader(204) return } @@ -147,7 +147,7 @@ func PostHook(c *gin.Context) { user, err := store_.GetUser(repo.UserID) if err != nil { log.Error().Msgf("failure to find repo owner %s. %s", repo.FullName, err) - c.AbortWithError(500, err) + _ = c.AbortWithError(500, err) return } @@ -171,14 +171,14 @@ func PostHook(c *gin.Context) { remoteYamlConfigs, err := configFetcher.Fetch(c) if err != nil { log.Error().Msgf("error: %s: cannot find %s in %s: %s", repo.FullName, repo.Config, build.Ref, err) - c.AbortWithError(404, err) + _ = c.AbortWithError(404, err) return } filtered, err := branchFiltered(build, remoteYamlConfigs) if err != nil { log.Error().Msgf("failure to parse yaml from hook for %s. %s", repo.FullName, err) - c.AbortWithError(400, err) + _ = c.AbortWithError(400, err) } if filtered { c.String(200, "Branch does not match restrictions defined in yaml") @@ -202,7 +202,7 @@ func PostHook(c *gin.Context) { err = store_.CreateBuild(build, build.Procs...) if err != nil { log.Error().Msgf("failure to save commit for %s. %s", repo.FullName, err) - c.AbortWithError(500, err) + _ = c.AbortWithError(500, err) return } @@ -211,7 +211,7 @@ func PostHook(c *gin.Context) { _, err := findOrPersistPipelineConfig(repo, build, remoteYamlConfig) if err != nil { log.Error().Msgf("failure to find or persist build config for %s. %s", repo.FullName, err) - c.AbortWithError(500, err) + _ = c.AbortWithError(500, err) return } } @@ -288,8 +288,12 @@ func PostHook(c *gin.Context) { } }() - publishToTopic(c, build, repo, model.Enqueued) - queueBuild(build, repo, buildItems) + if err := publishToTopic(c, build, repo, model.Enqueued); err != nil { + log.Error().Err(err).Msg("publishToTopic") + } + if err := queueBuild(build, repo, buildItems); err != nil { + log.Error().Err(err).Msg("queueBuild") + } } // TODO: parse yaml once and not for each filter function @@ -367,7 +371,7 @@ func findOrPersistPipelineConfig(repo *model.Repo, build *model.Build, remoteYam } // publishes message to UI clients -func publishToTopic(c *gin.Context, build *model.Build, repo *model.Repo, event model.EventType) { +func publishToTopic(c *gin.Context, build *model.Build, repo *model.Repo, event model.EventType) error { message := pubsub.Message{ Labels: map[string]string{ "repo": repo.FullName, @@ -381,10 +385,10 @@ func publishToTopic(c *gin.Context, build *model.Build, repo *model.Repo, event Repo: *repo, Build: buildCopy, }) - server.Config.Services.Pubsub.Publish(c, "topic/events", message) + return server.Config.Services.Pubsub.Publish(c, "topic/events", message) } -func queueBuild(build *model.Build, repo *model.Repo, buildItems []*shared.BuildItem) { +func queueBuild(build *model.Build, repo *model.Repo, buildItems []*shared.BuildItem) error { var tasks []*queue.Task for _, item := range buildItems { if item.Proc.State == model.StatusSkipped { @@ -408,10 +412,12 @@ func queueBuild(build *model.Build, repo *model.Repo, buildItems []*shared.Build Timeout: repo.Timeout, }) - server.Config.Services.Logs.Open(context.Background(), task.ID) + if err := server.Config.Services.Logs.Open(context.Background(), task.ID); err != nil { + return err + } tasks = append(tasks, task) } - server.Config.Services.Queue.PushAtOnce(context.Background(), tasks) + return server.Config.Services.Queue.PushAtOnce(context.Background(), tasks) } func taskIds(dependsOn []string, buildItems []*shared.BuildItem) (taskIds []string) { diff --git a/server/api/login.go b/server/api/login.go index e3db267673..f8bbcf5279 100644 --- a/server/api/login.go +++ b/server/api/login.go @@ -83,7 +83,7 @@ func HandleAuth(c *gin.Context) { // check the user's organization membership. if len(config.Orgs) != 0 { teams, terr := remote.Teams(c, tmpuser) - if terr != nil || config.IsMember(teams) == false { + if terr != nil || !config.IsMember(teams) { log.Error().Msgf("cannot verify team membership for %s.", u.Login) c.Redirect(303, "/login?error=access_denied") return @@ -120,7 +120,7 @@ func HandleAuth(c *gin.Context) { // check the user's organization membership. if len(config.Orgs) != 0 { teams, terr := remote.Teams(c, u) - if terr != nil || config.IsMember(teams) == false { + if terr != nil || !config.IsMember(teams) { log.Error().Msgf("cannot verify team membership for %s.", u.Login) c.Redirect(303, "/login?error=access_denied") return @@ -163,32 +163,32 @@ func GetLoginToken(c *gin.Context) { in := &tokenPayload{} err := c.Bind(in) if err != nil { - c.AbortWithError(http.StatusBadRequest, err) + _ = c.AbortWithError(http.StatusBadRequest, err) return } login, err := remote.Auth(c, in.Access, in.Refresh) if err != nil { - c.AbortWithError(http.StatusUnauthorized, err) + _ = c.AbortWithError(http.StatusUnauthorized, err) return } user, err := store_.GetUserLogin(login) if err != nil { - c.AbortWithError(http.StatusNotFound, err) + _ = c.AbortWithError(http.StatusNotFound, err) return } exp := time.Now().Add(server.Config.Server.SessionExpires).Unix() - token := token.New(token.SessToken, user.Login) - tokenstr, err := token.SignExpires(user.Hash, exp) + newToken := token.New(token.SessToken, user.Login) + tokenStr, err := newToken.SignExpires(user.Hash, exp) if err != nil { - c.AbortWithError(http.StatusInternalServerError, err) + _ = c.AbortWithError(http.StatusInternalServerError, err) return } c.JSON(http.StatusOK, &tokenPayload{ - Access: tokenstr, + Access: tokenStr, Expires: exp - time.Now().Unix(), }) } diff --git a/server/api/repo.go b/server/api/repo.go index 96a220fe8e..e5009fbd0b 100644 --- a/server/api/repo.go +++ b/server/api/repo.go @@ -22,6 +22,7 @@ import ( "github.com/gin-gonic/gin" "github.com/gorilla/securecookie" + "github.com/rs/zerolog/log" "github.com/woodpecker-ci/woodpecker/server" "github.com/woodpecker-ci/woodpecker/server/model" @@ -104,7 +105,7 @@ func PatchRepo(c *gin.Context) { in := new(model.RepoPatch) if err := c.Bind(in); err != nil { - c.AbortWithError(http.StatusBadRequest, err) + _ = c.AbortWithError(http.StatusBadRequest, err) return } @@ -143,7 +144,7 @@ func PatchRepo(c *gin.Context) { err := store_.UpdateRepo(repo) if err != nil { - c.AbortWithError(http.StatusInternalServerError, err) + _ = c.AbortWithError(http.StatusInternalServerError, err) return } @@ -158,7 +159,7 @@ func ChownRepo(c *gin.Context) { err := store_.UpdateRepo(repo) if err != nil { - c.AbortWithError(http.StatusInternalServerError, err) + _ = c.AbortWithError(http.StatusInternalServerError, err) return } c.JSON(http.StatusOK, repo) @@ -180,7 +181,7 @@ func GetRepoBranches(c *gin.Context) { branches, err := r.Branches(c, user, repo) if err != nil { - c.AbortWithError(http.StatusInternalServerError, err) + _ = c.AbortWithError(http.StatusInternalServerError, err) return } @@ -200,20 +201,20 @@ func DeleteRepo(c *gin.Context) { err := store_.UpdateRepo(repo) if err != nil { - c.AbortWithError(http.StatusInternalServerError, err) + _ = c.AbortWithError(http.StatusInternalServerError, err) return } if remove { err := store_.DeleteRepo(repo) if err != nil { - c.AbortWithError(http.StatusInternalServerError, err) + _ = c.AbortWithError(http.StatusInternalServerError, err) return } } if err := remote_.Deactivate(c, user, repo, server.Config.Server.Host); err != nil { - c.AbortWithError(http.StatusInternalServerError, err) + _ = c.AbortWithError(http.StatusInternalServerError, err) return } c.JSON(200, repo) @@ -241,26 +242,33 @@ func RepairRepo(c *gin.Context) { sig, ) - _ = remote_.Deactivate(c, user, repo, host) - err = remote_.Activate(c, user, repo, link) - if err != nil { + if err := remote_.Deactivate(c, user, repo, host); err != nil { + log.Trace().Err(err).Msgf("deactivate repo '%s' to repair failed", repo.FullName) + } + if err := remote_.Activate(c, user, repo, link); err != nil { c.String(500, err.Error()) return } from, err := remote_.Repo(c, user, repo.Owner, repo.Name) - if err == nil { - repo.Name = from.Name - repo.Owner = from.Owner - repo.FullName = from.FullName - repo.Avatar = from.Avatar - repo.Link = from.Link - repo.Clone = from.Clone - repo.IsSCMPrivate = from.IsSCMPrivate - if repo.IsSCMPrivate != from.IsSCMPrivate { - repo.ResetVisibility() - } - store_.UpdateRepo(repo) + if err != nil { + log.Error().Err(err).Msgf("get repo '%s/%s' from remote", repo.Owner, repo.Name) + c.AbortWithStatus(http.StatusInternalServerError) + return + } + repo.Name = from.Name + repo.Owner = from.Owner + repo.FullName = from.FullName + repo.Avatar = from.Avatar + repo.Link = from.Link + repo.Clone = from.Clone + repo.IsSCMPrivate = from.IsSCMPrivate + if repo.IsSCMPrivate != from.IsSCMPrivate { + repo.ResetVisibility() + } + if err := store_.UpdateRepo(repo); err != nil { + _ = c.AbortWithError(http.StatusInternalServerError, err) + return } c.Writer.WriteHeader(http.StatusOK) @@ -275,19 +283,19 @@ func MoveRepo(c *gin.Context) { to, exists := c.GetQuery("to") if !exists { err := fmt.Errorf("Missing required to query value") - c.AbortWithError(http.StatusInternalServerError, err) + _ = c.AbortWithError(http.StatusInternalServerError, err) return } owner, name, errParse := model.ParseRepo(to) if errParse != nil { - c.AbortWithError(http.StatusInternalServerError, errParse) + _ = c.AbortWithError(http.StatusInternalServerError, errParse) return } from, err := remote_.Repo(c, user, owner, name) if err != nil { - c.AbortWithError(http.StatusInternalServerError, err) + _ = c.AbortWithError(http.StatusInternalServerError, err) return } if !from.Perm.Admin { @@ -308,7 +316,7 @@ func MoveRepo(c *gin.Context) { errStore := store_.UpdateRepo(repo) if errStore != nil { - c.AbortWithError(http.StatusInternalServerError, errStore) + _ = c.AbortWithError(http.StatusInternalServerError, errStore) return } @@ -328,10 +336,10 @@ func MoveRepo(c *gin.Context) { sig, ) - // TODO: check if we should handle that error - remote_.Deactivate(c, user, repo, host) - err = remote_.Activate(c, user, repo, link) - if err != nil { + if err := remote_.Deactivate(c, user, repo, host); err != nil { + log.Trace().Err(err).Msgf("deactivate repo '%s' for move to activate later, got an error", repo.FullName) + } + if err := remote_.Activate(c, user, repo, link); err != nil { c.String(500, err.Error()) return } diff --git a/server/api/stream.go b/server/api/stream.go index c8e7146414..8b9a812b79 100644 --- a/server/api/stream.go +++ b/server/api/stream.go @@ -52,7 +52,7 @@ func EventStreamSSE(c *gin.Context) { } // ping the client - io.WriteString(rw, ": ping\n\n") + logWriteStringErr(io.WriteString(rw, ": ping\n\n")) flusher.Flush() log.Debug().Msg("user feed: connection opened") @@ -78,9 +78,10 @@ func EventStreamSSE(c *gin.Context) { }() go func() { - server.Config.Services.Pubsub.Subscribe(ctx, "topic/events", func(m pubsub.Message) { + err := server.Config.Services.Pubsub.Subscribe(ctx, "topic/events", func(m pubsub.Message) { defer func() { - recover() // fix #2480 + obj := recover() // fix #2480 // TODO: check if it's still needed + log.Trace().Msgf("pubsub subscribe recover return: %v", obj) }() name := m.Labels["repo"] priv := m.Labels["private"] @@ -93,6 +94,9 @@ func EventStreamSSE(c *gin.Context) { } } }) + if err != nil { + log.Error().Err(err).Msg("Subscribe failed") + } cancel() }() @@ -103,13 +107,13 @@ func EventStreamSSE(c *gin.Context) { case <-ctx.Done(): return case <-time.After(time.Second * 30): - io.WriteString(rw, ": ping\n\n") + logWriteStringErr(io.WriteString(rw, ": ping\n\n")) flusher.Flush() case buf, ok := <-eventc: if ok { - io.WriteString(rw, "data: ") - rw.Write(buf) - io.WriteString(rw, "\n\n") + logWriteStringErr(io.WriteString(rw, "data: ")) + logWriteStringErr(rw.Write(buf)) + logWriteStringErr(io.WriteString(rw, "\n\n")) flusher.Flush() } } @@ -130,7 +134,7 @@ func LogStreamSSE(c *gin.Context) { return } - io.WriteString(rw, ": ping\n\n") + logWriteStringErr(io.WriteString(rw, ": ping\n\n")) flusher.Flush() repo := session.Repo(c) @@ -144,18 +148,18 @@ func LogStreamSSE(c *gin.Context) { build, err := store_.GetBuildNumber(repo, buildn) if err != nil { log.Debug().Msgf("stream cannot get build number: %v", err) - io.WriteString(rw, "event: error\ndata: build not found\n\n") + logWriteStringErr(io.WriteString(rw, "event: error\ndata: build not found\n\n")) return } proc, err := store_.ProcFind(build, jobn) if err != nil { log.Debug().Msgf("stream cannot get proc number: %v", err) - io.WriteString(rw, "event: error\ndata: process not found\n\n") + logWriteStringErr(io.WriteString(rw, "event: error\ndata: process not found\n\n")) return } if proc.State != model.StatusRunning { log.Debug().Msg("stream not found.") - io.WriteString(rw, "event: error\ndata: stream not found\n\n") + logWriteStringErr(io.WriteString(rw, "event: error\ndata: stream not found\n\n")) return } @@ -174,9 +178,10 @@ func LogStreamSSE(c *gin.Context) { go func() { // TODO remove global variable - server.Config.Services.Logs.Tail(ctx, fmt.Sprint(proc.ID), func(entries ...*logging.Entry) { + err := server.Config.Services.Logs.Tail(ctx, fmt.Sprint(proc.ID), func(entries ...*logging.Entry) { defer func() { - recover() // fix #2480 + obj := recover() // fix #2480 // TODO: check if it's still needed + log.Trace().Msgf("pubsub subscribe recover return: %v", obj) }() for _, entry := range entries { select { @@ -187,8 +192,11 @@ func LogStreamSSE(c *gin.Context) { } } }) + if err != nil { + log.Error().Err(err).Msg("tail of logs failed") + } - io.WriteString(rw, "event: error\ndata: eof\n\n") + logWriteStringErr(io.WriteString(rw, "event: error\ndata: eof\n\n")) cancel() }() @@ -215,16 +223,16 @@ func LogStreamSSE(c *gin.Context) { case <-ctx.Done(): return case <-time.After(time.Second * 30): - io.WriteString(rw, ": ping\n\n") + logWriteStringErr(io.WriteString(rw, ": ping\n\n")) flusher.Flush() case buf, ok := <-logc: if ok { if id > last { - io.WriteString(rw, "id: "+strconv.Itoa(id)) - io.WriteString(rw, "\n") - io.WriteString(rw, "data: ") - rw.Write(buf) - io.WriteString(rw, "\n\n") + logWriteStringErr(io.WriteString(rw, "id: "+strconv.Itoa(id))) + logWriteStringErr(io.WriteString(rw, "\n")) + logWriteStringErr(io.WriteString(rw, "data: ")) + logWriteStringErr(rw.Write(buf)) + logWriteStringErr(io.WriteString(rw, "\n\n")) flusher.Flush() } id++ @@ -232,3 +240,9 @@ func LogStreamSSE(c *gin.Context) { } } } + +func logWriteStringErr(_ int, err error) { + if err != nil { + log.Error().Err(err).Caller(1).Msg("fail to write string") + } +} diff --git a/server/api/user.go b/server/api/user.go index da76bda8b1..0b178864ab 100644 --- a/server/api/user.go +++ b/server/api/user.go @@ -47,7 +47,10 @@ func GetFeed(c *gin.Context) { log.Debug().Msgf("sync begin: %s", user.Login) user.Synced = time.Now().Unix() - store_.UpdateUser(user) + if err := store_.UpdateUser(user); err != nil { + log.Error().Err(err).Msg("UpdateUser") + return + } config := ToConfig(c) @@ -138,7 +141,7 @@ func PostToken(c *gin.Context) { user := session.User(c) tokenString, err := token.New(token.UserToken, user.Login).Sign(user.Hash) if err != nil { - c.AbortWithError(http.StatusInternalServerError, err) + _ = c.AbortWithError(http.StatusInternalServerError, err) return } c.String(http.StatusOK, tokenString) @@ -158,7 +161,7 @@ func DeleteToken(c *gin.Context) { tokenString, err := token.New(token.UserToken, user.Login).Sign(user.Hash) if err != nil { - c.AbortWithError(http.StatusInternalServerError, err) + _ = c.AbortWithError(http.StatusInternalServerError, err) return } c.String(http.StatusOK, tokenString) diff --git a/server/api/z.go b/server/api/z.go index 664c836b94..6a7a851f6b 100644 --- a/server/api/z.go +++ b/server/api/z.go @@ -55,13 +55,13 @@ func SetLogLevel(c *gin.Context) { LogLevel string `json:"log-level"` }{} if err := c.Bind(&logLevel); err != nil { - c.AbortWithError(http.StatusBadRequest, err) + _ = c.AbortWithError(http.StatusBadRequest, err) return } lvl, err := zerolog.ParseLevel(logLevel.LogLevel) if err != nil { - c.AbortWithError(http.StatusBadRequest, err) + _ = c.AbortWithError(http.StatusBadRequest, err) return } diff --git a/server/grpc/rpc.go b/server/grpc/rpc.go index 57513bb8ac..e59b46e443 100644 --- a/server/grpc/rpc.go +++ b/server/grpc/rpc.go @@ -81,7 +81,9 @@ func (s *RPC) Next(c context.Context, filter rpc.Filter) (*rpc.Pipeline, error) err = json.Unmarshal(task.Data, pipeline) return pipeline, err } else { - s.Done(c, task.ID, rpc.State{}) + if err := s.Done(c, task.ID, rpc.State{}); err != nil { + log.Error().Err(err).Msgf("mark task '%s' done failed", task.ID) + } } } } @@ -151,7 +153,9 @@ func (s *RPC) Update(c context.Context, id string, state rpc.State) error { Repo: *repo, Build: *build, }) - s.pubsub.Publish(c, "topic/events", message) + if err := s.pubsub.Publish(c, "topic/events", message); err != nil { + log.Error().Err(err).Msg("can not publish proc list to") + } return nil } @@ -279,7 +283,9 @@ func (s *RPC) Init(c context.Context, id string, state rpc.State) error { Repo: *repo, Build: *build, }) - s.pubsub.Publish(c, "topic/events", message) + if err := s.pubsub.Publish(c, "topic/events", message); err != nil { + log.Error().Err(err).Msg("can not publish proc list to") + } }() _, err = shared.UpdateProcToStatusStarted(s.store, *proc, state) @@ -373,7 +379,9 @@ func isMultiPipeline(procs []*model.Proc) bool { func (s *RPC) Log(c context.Context, id string, line *rpc.Line) error { entry := new(logging.Entry) entry.Data, _ = json.Marshal(line) - s.logger.Write(c, id, entry) + if err := s.logger.Write(c, id, entry); err != nil { + log.Error().Err(err).Msgf("rpc server could not write to logger") + } return nil } @@ -416,9 +424,14 @@ func (s *RPC) updateRemoteStatus(ctx context.Context, repo *model.Repo, build *m user, err := s.store.GetUser(repo.UserID) if err == nil { if refresher, ok := s.remote.(remote.Refresher); ok { - ok, _ := refresher.Refresh(ctx, user) - if ok { - s.store.UpdateUser(user) + ok, err := refresher.Refresh(ctx, user) + if err != nil { + log.Error().Err(err).Msgf("grpc: refresh oauth token of user '%s' failed", user.Login) + } else if ok { + if err := s.store.UpdateUser(user); err != nil { + log.Error().Err(err).Msg("fail to save user to store after refresh oauth token") + } + } } uri := fmt.Sprintf("%s/%s/%d", server.Config.Server.Host, repo.FullName, build.Number) @@ -441,7 +454,9 @@ func (s *RPC) notify(c context.Context, repo *model.Repo, build *model.Build, pr Repo: *repo, Build: *build, }) - s.pubsub.Publish(c, "topic/events", message) + if err := s.pubsub.Publish(c, "topic/events", message); err != nil { + log.Error().Err(err).Msgf("grpc could not notify event: '%v'", message) + } } func createFilterFunc(filter rpc.Filter) (queue.Filter, error) { diff --git a/server/logging/log.go b/server/logging/log.go index 5ef0cf0642..ee7f711c46 100644 --- a/server/logging/log.go +++ b/server/logging/log.go @@ -132,11 +132,15 @@ func (l *log) Snapshot(c context.Context, path string, w io.Writer) error { return ErrNotFound } s.Lock() + defer s.Unlock() for _, entry := range s.list { - w.Write(entry.Data) - w.Write(cr) + if _, err := w.Write(entry.Data); err != nil { + return err + } + if _, err := w.Write(cr); err != nil { + return err + } } - s.Unlock() return nil } diff --git a/server/logging/log_test.go b/server/logging/log_test.go index 2c235297ee..cac7cbcd5c 100644 --- a/server/logging/log_test.go +++ b/server/logging/log_test.go @@ -5,6 +5,8 @@ import ( "sync" "testing" "time" + + "github.com/stretchr/testify/assert" ) func TestLogging(t *testing.T) { @@ -22,27 +24,27 @@ func TestLogging(t *testing.T) { ) logger := New() - logger.Open(ctx, testPath) + assert.NoError(t, logger.Open(ctx, testPath)) go func() { - logger.Tail(ctx, testPath, func(entry ...*Entry) { wg.Done() }) + assert.NoError(t, logger.Tail(ctx, testPath, func(entry ...*Entry) { wg.Done() })) }() go func() { - logger.Tail(ctx, testPath, func(entry ...*Entry) { wg.Done() }) + assert.NoError(t, logger.Tail(ctx, testPath, func(entry ...*Entry) { wg.Done() })) }() <-time.After(500 * time.Millisecond) wg.Add(4) go func() { - logger.Write(ctx, testPath, testEntry) - logger.Write(ctx, testPath, testEntry) + assert.NoError(t, logger.Write(ctx, testPath, testEntry)) + assert.NoError(t, logger.Write(ctx, testPath, testEntry)) }() wg.Wait() wg.Add(1) go func() { - logger.Tail(ctx, testPath, func(entry ...*Entry) { wg.Done() }) + assert.NoError(t, logger.Tail(ctx, testPath, func(entry ...*Entry) { wg.Done() })) }() <-time.After(500 * time.Millisecond) diff --git a/server/pubsub/pub_test.go b/server/pubsub/pub_test.go index 007f3192d3..9635494cf1 100644 --- a/server/pubsub/pub_test.go +++ b/server/pubsub/pub_test.go @@ -5,6 +5,8 @@ import ( "sync" "testing" "time" + + "github.com/stretchr/testify/assert" ) func TestPubsub(t *testing.T) { @@ -22,12 +24,12 @@ func TestPubsub(t *testing.T) { ) broker := New() - broker.Create(ctx, testTopic) + assert.NoError(t, broker.Create(ctx, testTopic)) go func() { - broker.Subscribe(ctx, testTopic, func(message Message) { wg.Done() }) + assert.NoError(t, broker.Subscribe(ctx, testTopic, func(message Message) { wg.Done() })) }() go func() { - broker.Subscribe(ctx, testTopic, func(message Message) { wg.Done() }) + assert.NoError(t, broker.Subscribe(ctx, testTopic, func(message Message) { wg.Done() })) }() <-time.After(500 * time.Millisecond) @@ -38,7 +40,7 @@ func TestPubsub(t *testing.T) { wg.Add(2) go func() { - broker.Publish(ctx, testTopic, testMessage) + assert.NoError(t, broker.Publish(ctx, testTopic, testMessage)) }() wg.Wait() @@ -80,9 +82,9 @@ func TestSubscriptionClosed(t *testing.T) { ) broker := New() - broker.Create(context.Background(), testTopic) + assert.NoError(t, broker.Create(context.Background(), testTopic)) go func() { - broker.Subscribe(context.Background(), testTopic, testCallback) + assert.NoError(t, broker.Subscribe(context.Background(), testTopic, testCallback)) wg.Done() }() @@ -93,7 +95,7 @@ func TestSubscriptionClosed(t *testing.T) { } wg.Add(1) - broker.Remove(context.Background(), testTopic) + assert.NoError(t, broker.Remove(context.Background(), testTopic)) wg.Wait() if _, ok := broker.(*publisher).topics[testTopic]; ok { diff --git a/server/queue/fifo_test.go b/server/queue/fifo_test.go index aa65f14362..f277d147b9 100644 --- a/server/queue/fifo_test.go +++ b/server/queue/fifo_test.go @@ -6,6 +6,8 @@ import ( "sync" "testing" "time" + + "github.com/stretchr/testify/assert" ) var noContext = context.Background() @@ -14,7 +16,7 @@ func TestFifo(t *testing.T) { want := &Task{ID: "1"} q := New() - q.Push(noContext, want) + assert.NoError(t, q.Push(noContext, want)) info := q.Info(noContext) if len(info.Pending) != 1 { t.Errorf("expect task in pending queue") @@ -37,7 +39,7 @@ func TestFifo(t *testing.T) { return } - q.Done(noContext, got.ID, StatusSuccess) + assert.NoError(t, q.Done(noContext, got.ID, StatusSuccess)) info = q.Info(noContext) if len(info.Pending) != 0 { t.Errorf("expect task removed from pending queue") @@ -54,7 +56,7 @@ func TestFifoExpire(t *testing.T) { q := New().(*fifo) q.extension = 0 - q.Push(noContext, want) + assert.NoError(t, q.Push(noContext, want)) info := q.Info(noContext) if len(info.Pending) != 1 { t.Errorf("expect task in pending queue") @@ -78,7 +80,7 @@ func TestFifoWait(t *testing.T) { want := &Task{ID: "1"} q := New().(*fifo) - q.Push(noContext, want) + assert.NoError(t, q.Push(noContext, want)) got, _ := q.Poll(noContext, func(*Task) bool { return true }) if got != want { @@ -89,12 +91,12 @@ func TestFifoWait(t *testing.T) { var wg sync.WaitGroup wg.Add(1) go func() { - q.Wait(noContext, got.ID) + assert.NoError(t, q.Wait(noContext, got.ID)) wg.Done() }() <-time.After(time.Millisecond) - q.Done(noContext, got.ID, StatusSuccess) + assert.NoError(t, q.Done(noContext, got.ID, StatusSuccess)) wg.Wait() } @@ -102,7 +104,7 @@ func TestFifoEvict(t *testing.T) { t1 := &Task{ID: "1"} q := New() - q.Push(noContext, t1) + assert.NoError(t, q.Push(noContext, t1)) info := q.Info(noContext) if len(info.Pending) != 1 { t.Errorf("expect task in pending queue") @@ -131,7 +133,7 @@ func TestFifoDependencies(t *testing.T) { } q := New().(*fifo) - q.PushAtOnce(noContext, []*Task{task2, task1}) + assert.NoError(t, q.PushAtOnce(noContext, []*Task{task2, task1})) got, _ := q.Poll(noContext, func(*Task) bool { return true }) if got != task1 { @@ -139,7 +141,7 @@ func TestFifoDependencies(t *testing.T) { return } - q.Done(noContext, got.ID, StatusSuccess) + assert.NoError(t, q.Done(noContext, got.ID, StatusSuccess)) got, _ = q.Poll(noContext, func(*Task) bool { return true }) if got != task2 { @@ -167,7 +169,7 @@ func TestFifoErrors(t *testing.T) { } q := New().(*fifo) - q.PushAtOnce(noContext, []*Task{task2, task3, task1}) + assert.NoError(t, q.PushAtOnce(noContext, []*Task{task2, task3, task1})) got, _ := q.Poll(noContext, func(*Task) bool { return true }) if got != task1 { @@ -175,7 +177,7 @@ func TestFifoErrors(t *testing.T) { return } - q.Error(noContext, got.ID, fmt.Errorf("exitcode 1, there was an error")) + assert.NoError(t, q.Error(noContext, got.ID, fmt.Errorf("exitcode 1, there was an error"))) got, _ = q.Poll(noContext, func(*Task) bool { return true }) if got != task2 { @@ -216,7 +218,7 @@ func TestFifoErrors2(t *testing.T) { } q := New().(*fifo) - q.PushAtOnce(noContext, []*Task{task2, task3, task1}) + assert.NoError(t, q.PushAtOnce(noContext, []*Task{task2, task3, task1})) for i := 0; i < 2; i++ { got, _ := q.Poll(noContext, func(*Task) bool { return true }) @@ -226,10 +228,10 @@ func TestFifoErrors2(t *testing.T) { } if got != task1 { - q.Done(noContext, got.ID, StatusSuccess) + assert.NoError(t, q.Done(noContext, got.ID, StatusSuccess)) } if got != task2 { - q.Error(noContext, got.ID, fmt.Errorf("exitcode 1, there was an error")) + assert.NoError(t, q.Error(noContext, got.ID, fmt.Errorf("exitcode 1, there was an error"))) } } @@ -263,7 +265,7 @@ func TestFifoErrorsMultiThread(t *testing.T) { } q := New().(*fifo) - q.PushAtOnce(noContext, []*Task{task2, task3, task1}) + assert.NoError(t, q.PushAtOnce(noContext, []*Task{task2, task3, task1})) obtainedWorkCh := make(chan *Task) @@ -291,7 +293,7 @@ func TestFifoErrorsMultiThread(t *testing.T) { return } else { task1Processed = true - q.Error(noContext, got.ID, fmt.Errorf("exitcode 1, there was an error")) + assert.NoError(t, q.Error(noContext, got.ID, fmt.Errorf("exitcode 1, there was an error"))) go func() { for { fmt.Printf("Worker spawned\n") @@ -306,7 +308,7 @@ func TestFifoErrorsMultiThread(t *testing.T) { return } else { task2Processed = true - q.Done(noContext, got.ID, StatusSuccess) + assert.NoError(t, q.Done(noContext, got.ID, StatusSuccess)) go func() { for { fmt.Printf("Worker spawned\n") @@ -356,14 +358,14 @@ func TestFifoTransitiveErrors(t *testing.T) { } q := New().(*fifo) - q.PushAtOnce(noContext, []*Task{task2, task3, task1}) + assert.NoError(t, q.PushAtOnce(noContext, []*Task{task2, task3, task1})) got, _ := q.Poll(noContext, func(*Task) bool { return true }) if got != task1 { t.Errorf("expect task1 returned from queue as task2 depends on it") return } - q.Error(noContext, got.ID, fmt.Errorf("exitcode 1, there was an error")) + assert.NoError(t, q.Error(noContext, got.ID, fmt.Errorf("exitcode 1, there was an error"))) got, _ = q.Poll(noContext, func(*Task) bool { return true }) if got != task2 { @@ -374,7 +376,7 @@ func TestFifoTransitiveErrors(t *testing.T) { t.Errorf("expect task2 should not run, since task1 failed") return } - q.Done(noContext, got.ID, StatusSkipped) + assert.NoError(t, q.Done(noContext, got.ID, StatusSkipped)) got, _ = q.Poll(noContext, func(*Task) bool { return true }) if got != task3 { @@ -406,12 +408,12 @@ func TestFifoCancel(t *testing.T) { } q := New().(*fifo) - q.PushAtOnce(noContext, []*Task{task2, task3, task1}) + assert.NoError(t, q.PushAtOnce(noContext, []*Task{task2, task3, task1})) _, _ = q.Poll(noContext, func(*Task) bool { return true }) - q.Error(noContext, task1.ID, fmt.Errorf("cancelled")) - q.Error(noContext, task2.ID, fmt.Errorf("cancelled")) - q.Error(noContext, task3.ID, fmt.Errorf("cancelled")) + assert.NoError(t, q.Error(noContext, task1.ID, fmt.Errorf("cancelled"))) + assert.NoError(t, q.Error(noContext, task2.ID, fmt.Errorf("cancelled"))) + assert.NoError(t, q.Error(noContext, task3.ID, fmt.Errorf("cancelled"))) info := q.Info(noContext) if len(info.Pending) != 0 { @@ -435,7 +437,7 @@ func TestFifoPause(t *testing.T) { q.Pause() t0 := time.Now() - q.Push(noContext, task1) + assert.NoError(t, q.Push(noContext, task1)) time.Sleep(20 * time.Millisecond) q.Resume() @@ -447,7 +449,7 @@ func TestFifoPause(t *testing.T) { } q.Pause() - q.Push(noContext, task1) + assert.NoError(t, q.Push(noContext, task1)) q.Resume() _, _ = q.Poll(noContext, func(*Task) bool { return true }) } @@ -459,7 +461,7 @@ func TestFifoPauseResume(t *testing.T) { q := New().(*fifo) q.Pause() - q.Push(noContext, task1) + assert.NoError(t, q.Push(noContext, task1)) q.Resume() _, _ = q.Poll(noContext, func(*Task) bool { return true }) @@ -484,7 +486,7 @@ func TestWaitingVsPending(t *testing.T) { } q := New().(*fifo) - q.PushAtOnce(noContext, []*Task{task2, task3, task1}) + assert.NoError(t, q.PushAtOnce(noContext, []*Task{task2, task3, task1})) got, _ := q.Poll(noContext, func(*Task) bool { return true }) @@ -493,7 +495,7 @@ func TestWaitingVsPending(t *testing.T) { t.Errorf("2 should wait on deps") } - q.Error(noContext, got.ID, fmt.Errorf("exitcode 1, there was an error")) + assert.NoError(t, q.Error(noContext, got.ID, fmt.Errorf("exitcode 1, there was an error"))) got, _ = q.Poll(noContext, func(*Task) bool { return true }) info = q.Info(noContext) diff --git a/server/queue/persistent.go b/server/queue/persistent.go index 2205b3c1a0..9e8f197c4d 100644 --- a/server/queue/persistent.go +++ b/server/queue/persistent.go @@ -18,6 +18,7 @@ package queue import ( "context" + "github.com/pkg/errors" "github.com/rs/zerolog/log" "github.com/woodpecker-ci/woodpecker/server/model" @@ -38,7 +39,9 @@ func WithTaskStore(q Queue, s model.TaskStore) Queue { DepStatus: make(map[string]string), }) } - q.PushAtOnce(context.Background(), toEnqueue) + if err := q.PushAtOnce(context.Background(), toEnqueue); err != nil { + log.Error().Err(err).Msg("PushAtOnce failed") + } return &persistentQueue{q, s} } @@ -49,35 +52,44 @@ type persistentQueue struct { // Push pushes a task to the tail of this queue. func (q *persistentQueue) Push(c context.Context, task *Task) error { - q.store.TaskInsert(&model.Task{ + if err := q.store.TaskInsert(&model.Task{ ID: task.ID, Data: task.Data, Labels: task.Labels, Dependencies: task.Dependencies, RunOn: task.RunOn, - }) + }); err != nil { + return err + } err := q.Queue.Push(c, task) if err != nil { - q.store.TaskDelete(task.ID) + if err2 := q.store.TaskDelete(task.ID); err2 != nil { + err = errors.Wrapf(err, "delete task '%s' failed: %v", task.ID, err2) + } } return err } // PushAtOnce pushes multiple tasks to the tail of this queue. func (q *persistentQueue) PushAtOnce(c context.Context, tasks []*Task) error { + // TODO: invent store.NewSession who return context including a session and make TaskInsert & TaskDelete use it for _, task := range tasks { - q.store.TaskInsert(&model.Task{ + if err := q.store.TaskInsert(&model.Task{ ID: task.ID, Data: task.Data, Labels: task.Labels, Dependencies: task.Dependencies, RunOn: task.RunOn, - }) + }); err != nil { + return err + } } err := q.Queue.PushAtOnce(c, tasks) if err != nil { for _, task := range tasks { - q.store.TaskDelete(task.ID) + if err := q.store.TaskDelete(task.ID); err != nil { + return err + } } } return err @@ -101,18 +113,20 @@ func (q *persistentQueue) Poll(c context.Context, f Filter) (*Task, error) { func (q *persistentQueue) Evict(c context.Context, id string) error { err := q.Queue.Evict(c, id) if err == nil { - q.store.TaskDelete(id) + return q.store.TaskDelete(id) } return err } // EvictAtOnce removes a pending task from the queue. func (q *persistentQueue) EvictAtOnce(c context.Context, ids []string) error { - err := q.Queue.EvictAtOnce(c, ids) - if err == nil { - for _, id := range ids { - q.store.TaskDelete(id) + if err := q.Queue.EvictAtOnce(c, ids); err != nil { + return err + } + for _, id := range ids { + if err := q.store.TaskDelete(id); err != nil { + return err } } - return err + return nil } diff --git a/server/remote/bitbucket/bitbucket.go b/server/remote/bitbucket/bitbucket.go index f3886cb7a5..f811daf504 100644 --- a/server/remote/bitbucket/bitbucket.go +++ b/server/remote/bitbucket/bitbucket.go @@ -239,7 +239,7 @@ func (c *config) Activate(ctx context.Context, u *model.User, r *model.Repo, lin if err != nil { return err } - c.Deactivate(ctx, u, r, link) + _ = c.Deactivate(ctx, u, r, link) return c.newClient(ctx, u).CreateHook(r.Owner, r.Name, &internal.Hook{ Active: true, diff --git a/server/remote/bitbucket/internal/client.go b/server/remote/bitbucket/internal/client.go index cd59d89ed9..03efe6e2fd 100644 --- a/server/remote/bitbucket/internal/client.go +++ b/server/remote/bitbucket/internal/client.go @@ -30,7 +30,6 @@ import ( const ( get = "GET" - put = "PUT" post = "POST" del = "DELETE" ) @@ -213,7 +212,7 @@ func (c *Client) do(rawurl, method string, in, out interface{}) (*string, error) // error response. if resp.StatusCode > http.StatusPartialContent { err := Error{} - json.NewDecoder(resp.Body).Decode(&err) + _ = json.NewDecoder(resp.Body).Decode(&err) err.Status = resp.StatusCode return nil, err } diff --git a/server/remote/bitbucket/parse.go b/server/remote/bitbucket/parse.go index 7d8b8240b3..a7961d81a7 100644 --- a/server/remote/bitbucket/parse.go +++ b/server/remote/bitbucket/parse.go @@ -28,13 +28,7 @@ const ( hookPush = "repo:push" hookPullCreated = "pullrequest:created" hookPullUpdated = "pullrequest:updated" - - changeBranch = "branch" - changeNamedBranch = "named_branch" - - stateMerged = "MERGED" - stateDeclined = "DECLINED" - stateOpen = "OPEN" + stateOpen = "OPEN" ) // parseHook parses a Bitbucket hook from an http.Request request and returns diff --git a/server/remote/coding/fixtures/handler.go b/server/remote/coding/fixtures/handler.go index 90028e51f9..144f56c114 100644 --- a/server/remote/coding/fixtures/handler.go +++ b/server/remote/coding/fixtures/handler.go @@ -100,11 +100,6 @@ func getDepot(c *gin.Context) { } } -func getProjects(c *gin.Context) { - c.Header("Content-Type", "application/json;charset=UTF-8") - c.String(200, fakeProjectsPayload) -} - func getFile(c *gin.Context) { c.Header("Content-Type", "application/json;charset=UTF-8") switch fmt.Sprintf("%s/%s/%s/%s", c.Param("gk"), c.Param("prj"), c.Param("ref"), c.Param("path")) { @@ -273,23 +268,6 @@ const projectNotFoundPayload = ` } ` -const fakeProjectsPayload = ` -{ - "code":0, - "data":{ - "list":{ - "owner_user_name":"demo1", - "name":"test1", - "icon":"/static/project_icon/scenery-5.png", - }, - "page":1, - "pageSize":1, - "totalPage":1, - "totalRow":1 - } -} -` - const fakeFilePayload = ` { "code":0, diff --git a/server/remote/gitea/fixtures/handler.go b/server/remote/gitea/fixtures/handler.go index 21e64647c2..399abdb004 100644 --- a/server/remote/gitea/fixtures/handler.go +++ b/server/remote/gitea/fixtures/handler.go @@ -76,7 +76,7 @@ func createRepoHook(c *gin.Context) { URL string `json:"url"` } `json:"config"` }{} - c.BindJSON(&in) + _ = c.BindJSON(&in) if in.Type != "gitea" || in.Conf.Type != "json" || in.Conf.URL != "http://localhost" { diff --git a/server/remote/github/convert.go b/server/remote/github/convert.go index 6a79ebb5c6..94c5f81f7a 100644 --- a/server/remote/github/convert.go +++ b/server/remote/github/convert.go @@ -115,18 +115,6 @@ func convertPerm(from *github.Repository) *model.Perm { } } -// convertTeamPerm is a helper function used to convert a GitHub organization -// permissions to the common Woodpecker permissions structure. -func convertTeamPerm(from *github.Membership) *model.Perm { - admin := false - if *from.Role == "admin" { - admin = true - } - return &model.Perm{ - Admin: admin, - } -} - // convertRepoList is a helper function used to convert a GitHub repository // list to the common Woodpecker repository structure. func convertRepoList(from []*github.Repository, private bool) []*model.Repo { diff --git a/server/remote/github/github.go b/server/remote/github/github.go index 5ebf734482..81e439a650 100644 --- a/server/remote/github/github.go +++ b/server/remote/github/github.go @@ -270,9 +270,9 @@ func (c *client) Dir(ctx context.Context, u *model.User, r *model.Repo, b *model for i := 0; i < len(data); i++ { select { - case err, _ := <-errc: + case err := <-errc: errors = append(errors, err) - case fileMeta, _ := <-fc: + case fileMeta := <-fc: files = append(files, fileMeta) } } @@ -464,7 +464,7 @@ func repoStatus(c context.Context, client *github.Client, r *model.Repo, b *mode return err } -var reDeploy = regexp.MustCompile(".+/deployments/(\\d+)") +var reDeploy = regexp.MustCompile(`.+/deployments/(\d+)`) func deploymentStatus(ctx context.Context, client *github.Client, r *model.Repo, b *model.Build, link string) error { matches := reDeploy.FindStringSubmatch(b.Link) diff --git a/server/remote/github/github_test.go b/server/remote/github/github_test.go index 6ea68a6dde..ba6dece3bb 100644 --- a/server/remote/github/github_test.go +++ b/server/remote/github/github_test.go @@ -150,11 +150,6 @@ var ( Token: "cfcd2084", } - fakeUserNoRepos = &model.User{ - Login: "octocat", - Token: "repos_not_found", - } - fakeRepo = &model.Repo{ Owner: "octocat", Name: "Hello-World", @@ -170,8 +165,4 @@ var ( Name: "repo_not_found", FullName: "test_name/repo_not_found", } - - fakeBuild = &model.Build{ - Commit: "9ecad50", - } ) diff --git a/server/remote/gogs/fixtures/handler.go b/server/remote/gogs/fixtures/handler.go index 4726c9c640..b67b78b21a 100644 --- a/server/remote/gogs/fixtures/handler.go +++ b/server/remote/gogs/fixtures/handler.go @@ -61,7 +61,7 @@ func createRepoHook(c *gin.Context) { URL string `json:"url"` } `json:"config"` }{} - c.BindJSON(&in) + _ = c.BindJSON(&in) if in.Type != "gogs" || in.Conf.Type != "json" || in.Conf.URL != "http://localhost" { diff --git a/server/router/middleware/session/repo.go b/server/router/middleware/session/repo.go index 18bef8e4af..33b9b068a6 100644 --- a/server/router/middleware/session/repo.go +++ b/server/router/middleware/session/repo.go @@ -104,7 +104,10 @@ func SetPerm() gin.HandlerFunc { perm.Repo = repo.FullName perm.UserID = user.ID perm.Synced = time.Now().Unix() - store_.PermUpsert(perm) + if err := store_.PermUpsert(perm); err != nil { + _ = c.AbortWithError(http.StatusInternalServerError, err) + return + } } } } diff --git a/server/router/middleware/session/user.go b/server/router/middleware/session/user.go index 7ba287aa3c..42a13edcd0 100644 --- a/server/router/middleware/session/user.go +++ b/server/router/middleware/session/user.go @@ -78,7 +78,7 @@ func MustAdmin() gin.HandlerFunc { case user == nil: c.String(401, "User not authorized") c.Abort() - case user.Admin == false: + case !user.Admin: c.String(403, "User not authorized") c.Abort() default: @@ -95,7 +95,7 @@ func MustRepoAdmin() gin.HandlerFunc { case user == nil: c.String(401, "User not authorized") c.Abort() - case perm.Admin == false: + case !perm.Admin: c.String(403, "User not authorized") c.Abort() default: diff --git a/server/router/middleware/token/token.go b/server/router/middleware/token/token.go index 19baf4acc5..b4d05d46d2 100644 --- a/server/router/middleware/token/token.go +++ b/server/router/middleware/token/token.go @@ -53,8 +53,10 @@ func Refresh(c *gin.Context) { // attempts to refresh the access token. If the // token is refreshed, we must also persist to the // database. - ok, _ = refresher.Refresh(c, user) - if ok { + ok, err := refresher.Refresh(c, user) + if err != nil { + log.Error().Err(err).Msgf("refresh oauth token of user '%s' failed", user.Login) + } else if ok { err := store.FromContext(c).UpdateUser(user) if err != nil { // we only log the error at this time. not sure diff --git a/server/shared/userSyncer.go b/server/shared/userSyncer.go index 4e5949c9ca..0e6745b86c 100644 --- a/server/shared/userSyncer.go +++ b/server/shared/userSyncer.go @@ -40,9 +40,8 @@ type Syncer struct { // synchronized with the local datastore. type FilterFunc func(*model.Repo) bool -// NamespaceFilter func NamespaceFilter(namespaces map[string]bool) FilterFunc { - if namespaces == nil || len(namespaces) == 0 { + if len(namespaces) == 0 { return noopFilter } return func(repo *model.Repo) bool { diff --git a/server/web/config.go b/server/web/config.go index c4e60c1eb3..f3a5ae277b 100644 --- a/server/web/config.go +++ b/server/web/config.go @@ -16,12 +16,13 @@ package web import ( "encoding/json" - "fmt" "net/http" "text/template" "time" "github.com/gin-gonic/gin" + "github.com/rs/zerolog/log" + "github.com/woodpecker-ci/woodpecker/server" "github.com/woodpecker-ci/woodpecker/server/router/middleware/session" "github.com/woodpecker-ci/woodpecker/shared/token" @@ -29,8 +30,6 @@ import ( ) func WebConfig(c *gin.Context) { - var err error - user := session.User(c) var csrf string @@ -65,10 +64,9 @@ func WebConfig(c *gin.Context) { c.Header("Content-Type", "text/javascript; charset=utf-8") tmpl := template.Must(template.New("").Funcs(funcMap).Parse(configTemplate)) - err = tmpl.Execute(c.Writer, configData) - if err != nil { - fmt.Println(err) - c.AbortWithError(http.StatusInternalServerError, nil) + if err := tmpl.Execute(c.Writer, configData); err != nil { + log.Error().Err(err).Msgf("could not execute template") + c.AbortWithStatus(http.StatusInternalServerError) } } diff --git a/server/web/web.go b/server/web/web.go index 189f6d46da..880d8478f2 100644 --- a/server/web/web.go +++ b/server/web/web.go @@ -23,6 +23,7 @@ import ( "time" "github.com/gin-gonic/gin" + "github.com/rs/zerolog/log" "github.com/woodpecker-ci/woodpecker/server/model" "github.com/woodpecker-ci/woodpecker/shared/token" @@ -67,8 +68,6 @@ func (w *website) Register(mux *gin.Engine) { } func (w *website) handleIndex(rw http.ResponseWriter, r *http.Request) { - rw.WriteHeader(200) - var csrf string var user, _ = ToUser(r.Context()) if user != nil { @@ -89,7 +88,13 @@ func (w *website) handleIndex(rw http.ResponseWriter, r *http.Request) { } rw.Header().Set("Content-Type", "text/html; charset=UTF-8") - w.tmpl.Execute(rw, params) + if err := w.tmpl.Execute(rw, params); err != nil { + rw.WriteHeader(http.StatusInternalServerError) + log.Error().Err(err).Msg("execute template") + return + } + + rw.WriteHeader(200) } func setupCache(h http.Handler) http.Handler { diff --git a/vendor/modules.txt b/vendor/modules.txt index f41ae35a39..0f23ea448a 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -436,6 +436,7 @@ github.com/pelletier/go-toml # github.com/phayes/checkstyle v0.0.0-20170904204023-bfd46e6a821d github.com/phayes/checkstyle # github.com/pkg/errors v0.9.1 +## explicit github.com/pkg/errors # github.com/pmezard/go-difflib v1.0.0 github.com/pmezard/go-difflib/difflib