diff --git a/.codecov.yml b/.codecov.yml index e00ce3d6..98286a3b 100644 --- a/.codecov.yml +++ b/.codecov.yml @@ -1,2 +1,7 @@ +coverage: + status: + project: off + patch: off + github_checks: annotations: false diff --git a/config/config.go b/config/config.go index f937396d..667fe279 100644 --- a/config/config.go +++ b/config/config.go @@ -8,10 +8,9 @@ import ( "sigs.k8s.io/yaml" ) +// Config is the config for the using of reviewbot. type Config struct { - GlobalDefaultConfig GlobalConfig `json:"globalDefaultConfig,omitempty"` - LogStorageConfig LogStorageConfig `json:"logStorageConfig,omitempty"` - ServerAddr string `json:"serverAddr,omitempty"` + GlobalDefaultConfig GlobalConfig `json:"globalDefaultConfig,omitempty"` // CustomConfig is the custom linter config. // e.g. @@ -39,12 +38,6 @@ type GlobalConfig struct { JavaStyleCheckRuleConfig string `json:"javastylecheckruleConfig,omitempty"` } -type LogStorageConfig struct { - CustomRemoteConfigs map[string]any `json:"customRemoteConfigs"` -} - -type GithubConfig struct{} - type Linter struct { // Enable is whether to enable this linter, if false, linter still run but not report. Enable *bool `json:"enable,omitempty"` diff --git a/internal/linters/github.go b/internal/linters/github.go index 82e1fe9b..f39fa999 100644 --- a/internal/linters/github.go +++ b/internal/linters/github.go @@ -153,11 +153,17 @@ func CreateGithubChecks(ctx context.Context, a Agent, lintErrs map[string][]Lint }, Output: &github.CheckRunOutput{ Title: github.String(fmt.Sprintf("%s found %d issues related to your changes", linterName, len(annotations))), - Summary: github.String(fmt.Sprintf("[full log ](%s)\n", a.LinterLogViewUrl) + Reference), Annotations: annotations, }, } + logURL := a.GenLogViewUrl() + if logURL == "" { + check.Output.Summary = github.String(Reference) + } else { + check.Output.Summary = github.String(fmt.Sprintf("[full log ](%s)\n", logURL) + Reference) + } + if len(annotations) > 0 { check.Conclusion = github.String("failure") } else { diff --git a/internal/linters/go/gomodcheck/gomodcheck.go b/internal/linters/go/gomodcheck/gomodcheck.go index 705e5df3..1d146cda 100644 --- a/internal/linters/go/gomodcheck/gomodcheck.go +++ b/internal/linters/go/gomodcheck/gomodcheck.go @@ -27,7 +27,6 @@ func goModCheckHandler(ctx context.Context, a linters.Agent) error { return err } return linters.Report(log, a, parsedOutput) - } func goModCheckOutput(log *xlog.Logger, a linters.Agent) (map[string][]linters.LinterOutput, error) { diff --git a/internal/linters/java/pmdcheck/pmdcheck.go b/internal/linters/java/pmdcheck/pmdcheck.go index 18f1525e..14be2759 100644 --- a/internal/linters/java/pmdcheck/pmdcheck.go +++ b/internal/linters/java/pmdcheck/pmdcheck.go @@ -48,6 +48,7 @@ func pmdCheckHandler(ctx context.Context, a linters.Agent) error { a.LinterConfig.Args = append(append(a.LinterConfig.Args, javaFiles...), "-R", checkrulePath) return linters.GeneralHandler(plog, a, linters.ExecRun, pmdcheckParser) } + func argsApply(log *xlog.Logger, a linters.Agent) linters.Agent { config := a.LinterConfig if len(config.Command) == 1 && config.Command[0] == linterName { diff --git a/internal/linters/linters.go b/internal/linters/linters.go index b3267256..9e78fc57 100644 --- a/internal/linters/linters.go +++ b/internal/linters/linters.go @@ -100,10 +100,12 @@ type LinterOutput struct { // Agent knows necessary information in order to run linters. type Agent struct { + // ID each linter execution has a unique id. + ID string // Context is the context of the agent. Context context.Context - // LogStorages is the way to contral log storage - LogStorage storage.Storage + // Storage knows how to store and retrieve the linter logs. + Storage storage.Storage // Runner is the way to run the linter. like docker, local, etc. Runner runner.Runner // GitHubClient is the GitHub client. @@ -120,12 +122,11 @@ type Agent struct { LinterName string // RepoDir is the repo directory. RepoDir string - // LinterUuid is the uinque id of onece linter exec . - LinterUuid string - // LinterLogStoragePath is the path of log storage - LinterLogStoragePath string - // LinterLogStoragePath is the path of log storage - LinterLogViewUrl string + + // GenLogKey generates the log key. + GenLogKey func() string + // GenLogViewUrl generates the log view url. + GenLogViewUrl func() string } const CommentFooter = ` @@ -181,12 +182,9 @@ func ExecRun(a Agent) ([]byte, error) { return nil, fmt.Errorf("failed to read linter output: %w", err) } - ctx := context.Background() - err = a.LogStorage.Writer(ctx, a.LinterLogStoragePath, output) + err = a.Storage.Write(a.Context, a.GenLogKey(), output) if err != nil { log.Errorf("write to storage was failed %v", err) - } else { - log.Info("write to storage was successful") } return output, nil diff --git a/internal/linters/linters_test.go b/internal/linters/linters_test.go index 6b637a32..cd9464c1 100644 --- a/internal/linters/linters_test.go +++ b/internal/linters/linters_test.go @@ -20,11 +20,13 @@ import ( "context" "errors" "fmt" + "os" "reflect" "testing" "github.com/qiniu/reviewbot/config" "github.com/qiniu/reviewbot/internal/runner" + "github.com/qiniu/reviewbot/internal/storage" "github.com/qiniu/x/xlog" ) @@ -227,6 +229,12 @@ func TestExecRun(t *testing.T) { } for _, tc := range tcs { t.Run(tc.id, func(t *testing.T) { + storage, err := storage.NewLocalStorage(os.TempDir()) + if err != nil { + t.Errorf("failed to create local storage: %v", err) + } + tc.input.Storage = storage + tc.input.GenLogKey = func() string { return "test" } tc.input.Runner = runner.NewLocalRunner() tc.input.LinterConfig.Modifier = config.NewBaseModifier() tc.input.Context = context.WithValue(context.Background(), config.EventGUIDKey, "test") diff --git a/internal/runner/runner.go b/internal/runner/runner.go index 846a1ed8..6b715cb8 100644 --- a/internal/runner/runner.go +++ b/internal/runner/runner.go @@ -18,14 +18,17 @@ import ( "github.com/qiniu/x/xlog" ) +// Runner defines the interface for how to run the linter. type Runner interface { + // Prepare prepares the linter for running. Prepare(ctx context.Context, cfg *config.Linter) error + + // Run runs the linter and returns the output. Run(ctx context.Context, cfg *config.Linter) (io.ReadCloser, error) } // LocalRunner is a runner that runs the linter locally. -type LocalRunner struct { -} +type LocalRunner struct{} func NewLocalRunner() Runner { return &LocalRunner{} diff --git a/internal/storage/file.go b/internal/storage/file.go index 3f9e0b2b..03a816c9 100644 --- a/internal/storage/file.go +++ b/internal/storage/file.go @@ -5,26 +5,33 @@ import ( "fmt" "os" "path/filepath" - "sync" "github.com/qiniu/x/log" ) type LocalStorage struct { - mu sync.Mutex + rootDir string } -func NewLocalStorage() Storage { - return &LocalStorage{} +func NewLocalStorage(rootDir string) (Storage, error) { + rootDir, err := filepath.Abs(rootDir) + if err != nil { + return nil, fmt.Errorf("failed to get absolute path: %w", err) + } + if err := os.MkdirAll(rootDir, 0o755); err != nil { + return nil, fmt.Errorf("failed to make log dir: %w", err) + } + return &LocalStorage{rootDir: rootDir}, nil } -func (*LocalStorage) Writer(ctx context.Context, path string, content []byte) error { - if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { +func (l *LocalStorage) Write(ctx context.Context, path string, content []byte) error { + logFile := filepath.Join(l.rootDir, path, DefaultLogName) + log.Infof("writing log to %s", logFile) + if err := os.MkdirAll(filepath.Dir(logFile), 0o755); err != nil { log.Errorf("failed to make log dir: %v", err) } - dir, _ := os.Getwd() - log.Infof("pwd: %s", dir) - file, err := os.OpenFile(path, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0644) + + file, err := os.OpenFile(logFile, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0o644) if err != nil { return fmt.Errorf("failed to open file: %w", err) } @@ -42,14 +49,12 @@ func (*LocalStorage) Writer(ctx context.Context, path string, content []byte) er return nil } -func (g *LocalStorage) Reader(ctx context.Context, path string) ([]byte, error) { - filePath := path +func (l *LocalStorage) Read(ctx context.Context, path string) ([]byte, error) { + filePath := filepath.Join(l.rootDir, path, DefaultLogName) if _, err := os.Stat(filePath); os.IsNotExist(err) { return nil, fmt.Errorf("file does not exist: %s", filePath) } - g.mu.Lock() - defer g.mu.Unlock() data, err := os.ReadFile(filePath) if err != nil { return nil, fmt.Errorf("error reading file: %w", err) diff --git a/internal/storage/git.go b/internal/storage/git.go new file mode 100644 index 00000000..60549f85 --- /dev/null +++ b/internal/storage/git.go @@ -0,0 +1,19 @@ +package storage + +import ( + "context" +) + +type GitStorage struct{} + +func NewGitStorage() (Storage, error) { + return &GitStorage{}, nil +} + +func (g *GitStorage) Write(ctx context.Context, key string, content []byte) error { + return nil +} + +func (g *GitStorage) Read(ctx context.Context, key string) ([]byte, error) { + return nil, nil +} diff --git a/internal/storage/github.go b/internal/storage/github.go deleted file mode 100644 index 00060c26..00000000 --- a/internal/storage/github.go +++ /dev/null @@ -1,21 +0,0 @@ -package storage - -import ( - "context" - - "github.com/qiniu/reviewbot/config" -) - -type GithubStorage struct{} - -func NewGitubStorage(cfg config.GithubConfig) Storage { - return &GithubStorage{} -} - -func (g *GithubStorage) Writer(ctx context.Context, path string, content []byte) error { - return nil -} - -func (g *GithubStorage) Reader(ctx context.Context, path string) ([]byte, error) { - return []byte{}, nil -} diff --git a/internal/storage/s3.go b/internal/storage/s3.go new file mode 100644 index 00000000..cc8dcc25 --- /dev/null +++ b/internal/storage/s3.go @@ -0,0 +1,21 @@ +package storage + +import "context" + +type S3Storage struct { + credential string +} + +func NewS3Storage(credential string) (Storage, error) { + return &S3Storage{ + credential: credential, + }, nil +} + +func (s *S3Storage) Write(ctx context.Context, key string, content []byte) error { + return nil +} + +func (s *S3Storage) Read(ctx context.Context, key string) ([]byte, error) { + return nil, nil +} diff --git a/internal/storage/storage.go b/internal/storage/storage.go index e5e627bf..8c4d7919 100644 --- a/internal/storage/storage.go +++ b/internal/storage/storage.go @@ -3,7 +3,13 @@ package storage import "context" type Storage interface { - Writer(ctx context.Context, path string, content []byte) error + // Write writes the content to the specified key. + Write(ctx context.Context, key string, content []byte) error - Reader(ctx context.Context, path string) ([]byte, error) + // Read reads the content from the specified key. + Read(ctx context.Context, key string) ([]byte, error) } + +const ( + DefaultLogName = "log.txt" +) diff --git a/internal/storage/storage_test.go b/internal/storage/storage_test.go new file mode 100644 index 00000000..284bb389 --- /dev/null +++ b/internal/storage/storage_test.go @@ -0,0 +1,65 @@ +package storage_test + +import ( + "context" + "os" + "path/filepath" + "testing" + + "github.com/qiniu/reviewbot/internal/storage" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestFileStorage(t *testing.T) { + tempDir, err := os.MkdirTemp("", "file_storage_test") + require.NoError(t, err) + defer os.RemoveAll(tempDir) + fs, err := storage.NewLocalStorage(tempDir) + require.NoError(t, err) + + t.Run("Write and Read", func(t *testing.T) { + ctx := context.Background() + content := []byte("测试内容") + path := "test.txt" + err := fs.Write(ctx, path, content) + require.NoError(t, err) + readContent, err := fs.Read(ctx, path) + require.NoError(t, err) + assert.Equal(t, content, readContent) + }) + + t.Run("Read Non-existent File", func(t *testing.T) { + ctx := context.Background() + _, err := fs.Read(ctx, "non_existent.txt") + require.Error(t, err) + }) + + t.Run("Write to Non-existent Directory", func(t *testing.T) { + ctx := context.Background() + content := []byte("测试内容") + path := filepath.Join("non", "existent", "dir", "test.txt") + + err := fs.Write(ctx, path, content) + require.NoError(t, err) + + readContent, err := fs.Read(ctx, path) + require.NoError(t, err) + require.Equal(t, content, readContent) + }) + + t.Run("Overwrite Existing File", func(t *testing.T) { + ctx := context.Background() + path := "overwrite.txt" + err := fs.Write(ctx, path, []byte("原始内容")) + require.NoError(t, err) + + newContent := []byte("新内容") + err = fs.Write(ctx, path, newContent) + require.NoError(t, err) + + readContent, err := fs.Read(ctx, path) + require.NoError(t, err) + require.Equal(t, newContent, readContent) + }) +} diff --git a/main.go b/main.go index d780e4f1..12de8173 100644 --- a/main.go +++ b/main.go @@ -17,7 +17,6 @@ package main import ( - "context" "errors" "expvar" "flag" @@ -63,6 +62,14 @@ type options struct { appID int64 installationID int64 appPrivateKey string + + // log storage dir for local storage + logDir string + // s3 credential config + s3Credential string + // server addr which is used to generate the log view url + // e.g. https://domain + serverAddr string } func (o options) Validate() error { @@ -95,7 +102,12 @@ func gatherOptions() options { fs.Int64Var(&o.appID, "app-id", 0, "github app id") fs.Int64Var(&o.installationID, "app-installation-id", 0, "github app installation id") fs.StringVar(&o.appPrivateKey, "app-private-key", "", "github app private key") - fs.Parse(os.Args[1:]) + fs.StringVar(&o.logDir, "log-dir", "/tmp", "log storage dir for local storage") + fs.StringVar(&o.serverAddr, "server-addr", "", "server addr which is used to generate the log view url") + err := fs.Parse(os.Args[1:]) + if err != nil { + log.Fatalf("failed to parse flags: %v", err) + } return o } @@ -146,13 +158,28 @@ func main() { appID: o.appID, appPrivateKey: o.appPrivateKey, debug: o.debug, + serverAddr: o.serverAddr, } go s.initDockerRunner() + // Prioritize using S3 storage if s3 credential is provided + if o.s3Credential != "" { + s.storage, err = storage.NewS3Storage(o.s3Credential) + if err != nil { + log.Fatalf("failed to create s3 storage: %v", err) + } + } else { + // Fallback to local storage + s.storage, err = storage.NewLocalStorage(o.logDir) + if err != nil { + log.Fatalf("failed to create local storage: %v", err) + } + } + mux := http.NewServeMux() mux.Handle("/", s) - mux.Handle("/view/", HandleView(storage.NewLocalStorage())) + mux.Handle("/view/", http.HandlerFunc(s.HandleView)) mux.Handle("/metrics", promhttp.Handler()) log.Infof("listening on port %d", o.port) @@ -176,19 +203,16 @@ func main() { log.Fatal(http.ListenAndServe(fmt.Sprintf(":%d", o.port), mux)) } -func HandleView(ls storage.Storage) http.HandlerFunc { - return func(w http.ResponseWriter, r *http.Request) { - uri := r.URL.Path[len("/view/"):] - ctx := context.Background() - linterLog, err := ls.Reader(ctx, uri) - - if err != nil { - http.Error(w, fmt.Sprintf("Log not found: %v", err), http.StatusNotFound) - return - } - - if _, err = w.Write(linterLog); err != nil { - log.Warnf("Error writing log, %v", err) - } +func (s *Server) HandleView(w http.ResponseWriter, r *http.Request) { + path := r.URL.Path[len("/view/"):] + // TODO(CarlJi): url decode + contents, err := s.storage.Read(r.Context(), path) + if err != nil { + http.Error(w, fmt.Sprintf("Log not found: %v", err), http.StatusNotFound) + return } + + w.Header().Set("Content-Type", "text/html") + w.WriteHeader(http.StatusOK) + w.Write(contents) } diff --git a/server.go b/server.go index 0d24696b..66071f7d 100644 --- a/server.go +++ b/server.go @@ -43,7 +43,12 @@ type Server struct { gitClientFactory gitv2.ClientFactory config config.Config + // server addr which is used to generate the log view url + // e.g. https://domain + serverAddr string + dockerRunner runner.Runner + storage storage.Storage webhookSecret []byte @@ -102,7 +107,6 @@ func (s *Server) ServeHTTP(w http.ResponseWriter, r *http.Request) { payload, err := github.ValidatePayload(r, s.webhookSecret) if err != nil { - log.Errorf("validate payload failed: %v", err) http.Error(w, err.Error(), http.StatusBadRequest) return } @@ -265,6 +269,7 @@ func (s *Server) handle(ctx context.Context, event *github.PullRequestEvent) err LinterName: name, RepoDir: r.Directory(), Context: ctx, + ID: uuid.New().String(), } if !linters.LinterRelated(name, agent) { @@ -277,28 +282,18 @@ func (s *Server) handle(ctx context.Context, event *github.PullRequestEvent) err r = s.dockerRunner } agent.Runner = r - - logStorageConfig := s.config.LogStorageConfig - if len(logStorageConfig.CustomRemoteConfigs) == 0 { - agent.LogStorage = storage.NewLocalStorage() - } else { - for remoteName, storageConfig := range logStorageConfig.CustomRemoteConfigs { - switch remoteName { - case "github": - agent.LogStorage = storage.NewGitubStorage(storageConfig.(config.GithubConfig)) - default: - log.Warnf("%s was wrong storageType", remoteName) - } + agent.Storage = s.storage + agent.GenLogKey = func() string { + return fmt.Sprintf("%s/%s/%s", agent.LinterName, agent.PullRequestEvent.Repo.GetFullName(), agent.ID) + } + agent.GenLogViewUrl = func() string { + // if serverAddr is not provided, return empty string + if s.serverAddr == "" { + return "" } + return s.serverAddr + "/view/" + agent.GenLogKey() } - agent.LinterUuid = uuid.New().String() - agent.LinterLogStoragePath = fmt.Sprintf("linter-logs/%s/%d/%s/log-build.txt", *agent.PullRequestEvent.Repo.Name, *agent.PullRequestEvent.Number, agent.LinterUuid) - agent.LinterLogViewUrl = "http://" + s.config.ServerAddr + "/view/" + agent.LinterLogStoragePath - - // s.config.ServerAddr = "localhost:8888" - // log.Debugf("---------LinterLogViewUrl--------- : %s", agent.LinterLogViewUrl) - if err := fn(ctx, agent); err != nil { log.Errorf("failed to run linter: %v", err) // continue to run other linters