ci: make go lint fail build (grafana#427)

* benchmark: cleanup

* server: clean up lint errors

* storage: clean up lint errors

* cli: clean up lint errors

* config: clean up lint errors

* cmd: clean up lint errors

* pkg misc: clean up lint errors

* benchmark: clean up lint errors

* scripts: clean up lint errors

* revive: set severity to warning

also introduce error just to test

* revive: ignore ./pkg/agent/pprof/ in ci

* lint: update based on anton's suggestions

* chore: fix flaky query test (grafana#430)

* benchmark: cleanup

* server: clean up lint errors

* storage: clean up lint errors

* cli: clean up lint errors

* config: clean up lint errors

* cmd: clean up lint errors

* pkg misc: clean up lint errors

* benchmark: clean up lint errors

* scripts: clean up lint errors

* revive: set severity to warning

also introduce error just to test

* revive: ignore ./pkg/agent/pprof/ in ci

* lint: update based on anton's suggestions

* benchmark: fix URL in template

Co-authored-by: Anton Kolesnikov <anton.e.kolesnikov@gmail.com>

.github/workflows/lint-go.yml

@@ -16,4 +16,5 @@ jobs:
         uses: petethepig/revive-action@v5
         with:
           config: revive.toml
-          exclude: 'vendor/...'
+          # same as in the `lint` rule of Makefile
+          exclude: 'vendor/...;./pkg/agent/pprof/...'

benchmark/cmd/command/root.go

@@ -12,7 +12,7 @@ import (
 	"github.com/spf13/cobra"
 )
 
-func newRootCmd(cfg *config.Config) *cobra.Command {
+func newRootCmd(*config.Config) *cobra.Command {
 	rootCmd := &cobra.Command{
 		Use: "pyrobench [flags] <subcommand>",
 	}

benchmark/cmd/main.go

@@ -10,11 +10,7 @@ import (
 
 func main() {
 	if err := command.Initialize(); err != nil {
-		fatalf("%s %v\n\n", color.RedString("Error:"), err)
+		fmt.Fprintf(os.Stderr, "%s %v\n\n", color.RedString("Error:"), err)
+		os.Exit(1)
 	}
 }
-
-func fatalf(format string, args ...interface{}) {
-	_, _ = fmt.Fprintf(os.Stderr, format, args...)
-	os.Exit(1)
-}

benchmark/internal/cireport/imagereport.go

@@ -28,7 +28,7 @@ type DashboardScreenshotter interface {
 	AllPanels(ctx context.Context, dashboardUID string, from int64, to int64) ([]Panel, error)
 }
 
-type imageReporter struct {
+type ImageReporter struct {
 	uploader      Uploader
 	screenshotter DashboardScreenshotter
 }
@@ -50,7 +50,7 @@ func ImageReportCLI(cfg config.ImageReport) (string, error) {
 
 	return r.Report(
 		context.Background(),
-		cfg.DashboardUid,
+		cfg.DashboardUID,
 		cfg.UploadDest,
 		from,
 		to,
@@ -59,17 +59,17 @@ func ImageReportCLI(cfg config.ImageReport) (string, error) {
 
 type screenshotPanel struct {
 	Title string
-	Url   string
+	URL   string
 }
 
-func NewImageReporter(screenshotter GrafanaScreenshotter, uploader Uploader) *imageReporter {
-	return &imageReporter{
+func NewImageReporter(screenshotter GrafanaScreenshotter, uploader Uploader) *ImageReporter {
+	return &ImageReporter{
 		uploader,
 		&screenshotter,
 	}
 }
 
-func (r *imageReporter) Report(ctx context.Context, dashboardUID string, dir string, from int64, to int64) (string, error) {
+func (r *ImageReporter) Report(ctx context.Context, dashboardUID string, dir string, from int64, to int64) (string, error) {
 	// screenshot all panes
 	logrus.Debug("taking screenshot of all panels")
 	panels, err := r.screenshotter.AllPanels(ctx, dashboardUID, from, to)
@@ -87,14 +87,14 @@ func (r *imageReporter) Report(ctx context.Context, dashboardUID string, dir str
 		i := i
 
 		g.Go(func() error {
-			publicUrl, err := r.uploader.WriteFile(filename(dir, p.Title), p.Data)
+			publicURL, err := r.uploader.WriteFile(filename(dir, p.Title), p.Data)
 			if err != nil {
 				return err
 			}
 
 			// TODO lock this?
 			sp[i].Title = p.Title
-			sp[i].Url = publicUrl
+			sp[i].URL = publicURL
 			return nil
 		})
 	}
@@ -104,7 +104,7 @@ func (r *imageReporter) Report(ctx context.Context, dashboardUID string, dir str
 	}
 
 	logrus.Debug("generating markdown report")
-	return r.template(sp)
+	return r.tpl(sp)
 }
 
 func filename(dir string, s string) string {
@@ -131,7 +131,7 @@ func normalizeWord(s string) string {
 	return result
 }
 
-func (r *imageReporter) template(panels []screenshotPanel) (string, error) {
+func (*ImageReporter) tpl(panels []screenshotPanel) (string, error) {
 	var tpl bytes.Buffer
 
 	data := struct {
@@ -156,10 +156,10 @@ func (r *imageReporter) template(panels []screenshotPanel) (string, error) {
 	return tpl.String(), nil
 }
 
-func decideTimestamp(fromInt, toInt int) (int64, int64) {
+func decideTimestamp(fromInt, toInt int) (from, to int64) {
 	now := time.Now()
-	from := int64(fromInt)
-	to := int64(toInt)
+	from = int64(fromInt)
+	to = int64(toInt)
 
 	// set defaults if appropriate
 	if to == 0 {
@@ -186,7 +186,7 @@ func decideUploader(uploadType string, uploadBucket string) (Uploader, error) {
 			return nil, err
 		}
 	case "fs":
-		uploader = NewFsWriter()
+		uploader = &FsWriter{}
 	default:
 		return nil, fmt.Errorf("invalid upload type: '%s'", uploadType)
 	}

benchmark/internal/cireport/metareport.go

@@ -9,13 +9,13 @@ import (
 	"github.com/sirupsen/logrus"
 )
 
-type metaReport struct {
+type MetaReport struct {
 	allowlist map[string]bool
 }
 
 // NewMetaReport creates a meta report
 // the allowlist parameter refers to which keys are valid
-func NewMetaReport(allowlist []string) (*metaReport, error) {
+func NewMetaReport(allowlist []string) (*MetaReport, error) {
 	if len(allowlist) <= 0 {
 		return nil, fmt.Errorf("at least one item should be allowed")
 	}
@@ -25,7 +25,7 @@ func NewMetaReport(allowlist []string) (*metaReport, error) {
 		a[v] = true
 	}
 
-	return &metaReport{
+	return &MetaReport{
 		allowlist: a,
 	}, nil
 }
@@ -36,7 +36,7 @@ type meta struct {
 }
 
 // Report generates a markdown report
-func (mr *metaReport) Report(vars []string) (string, error) {
+func (mr *MetaReport) Report(vars []string) (string, error) {
 	if len(vars) <= 0 {
 		return "", fmt.Errorf("at least one item should be reported")
 	}
@@ -62,15 +62,15 @@ func (mr *metaReport) Report(vars []string) (string, error) {
 	}
 
 	logrus.Debug("generating template")
-	report, err := mr.template(m)
+	report, err := mr.tpl(m)
 	if err != nil {
 		return "", err
 	}
 
 	return report, nil
 }
 
-func (mr *metaReport) breakOnEqual(s string) (string, string, error) {
+func (*MetaReport) breakOnEqual(s string) (key string, value string, err error) {
 	split := strings.Split(s, "=")
 	if len(split) != 2 {
 		return "", "", fmt.Errorf("expect value in format A=B")
@@ -83,7 +83,7 @@ func (mr *metaReport) breakOnEqual(s string) (string, string, error) {
 	return split[0], split[1], nil
 }
 
-func (mr *metaReport) validate(m []meta) error {
+func (mr *MetaReport) validate(m []meta) error {
 	for _, v := range m {
 		if _, ok := mr.allowlist[v.Key]; !ok {
 			return fmt.Errorf("key is not allowed: '%s'", v.Key)
@@ -93,7 +93,7 @@ func (mr *metaReport) validate(m []meta) error {
 	return nil
 }
 
-func (mr *metaReport) template(m []meta) (string, error) {
+func (*MetaReport) tpl(m []meta) (string, error) {
 	var tpl bytes.Buffer
 
 	data := struct {

benchmark/internal/cireport/resources/image-report.gotpl

@@ -4,7 +4,7 @@
 {{ range .Panels }}
 <details>
   <summary>{{ .Title }}</summary>
-  <img src="{{ .Url }}?{{ $root.Ts }}" alt="{{ .Title }}" />
+  <img src="{{ .URL }}?{{ $root.Ts }}" alt="{{ .Title }}" />
 {{ printf "\n" }}
 </details>
 {{ end }}

benchmark/internal/cireport/screenshotter.go

@@ -20,7 +20,7 @@ type GrafanaScreenshotter struct {
 
 type ScreenshotPanelConfig struct {
 	DashboardUID string
-	PanelId      int
+	PanelID      int
 	From         int64
 	To           int64
 	Width        int
@@ -39,7 +39,7 @@ func (gs *GrafanaScreenshotter) ScreenshotPanel(ctx context.Context, cfg Screens
 	q := req.URL.Query()
 	q.Add("from", strconv.FormatInt(cfg.From, 10))
 	q.Add("to", strconv.FormatInt(cfg.To, 10))
-	q.Add("panelId", strconv.Itoa(cfg.PanelId))
+	q.Add("panelId", strconv.Itoa(cfg.PanelID))
 
 	req.URL.RawQuery = q.Encode()
 
@@ -131,7 +131,7 @@ func (gs *GrafanaScreenshotter) AllPanels(ctx context.Context, dashboardUID stri
 			d, err := gs.ScreenshotPanel(ctx,
 				ScreenshotPanelConfig{
 					DashboardUID: dashboardUID,
-					PanelId:      id,
+					PanelID:      id,
 					From:         from,
 					To:           to,
 				})

benchmark/internal/cireport/tablereport.go

@@ -46,12 +46,12 @@ type QueriesConfig struct {
 
 	Queries []Query `yaml:"queries"`
 }
-type tableReport struct {
+type TableReport struct {
 	q Querier
 }
 
-func NewTableReport(q Querier) *tableReport {
-	return &tableReport{
+func NewTableReport(q Querier) *TableReport {
+	return &TableReport{
 		q,
 	}
 }
@@ -75,7 +75,7 @@ func TableReportCli(q Querier, queriesFile string) (string, error) {
 }
 
 // TableReport reports query results from prometheus in markdown format
-func (r *tableReport) Report(ctx context.Context, qCfg *QueriesConfig) (string, error) {
+func (r *TableReport) Report(ctx context.Context, qCfg *QueriesConfig) (string, error) {
 	// TODO: treat each error individually?
 	g, ctx := errgroup.WithContext(ctx)
 
@@ -149,18 +149,20 @@ func formatDiff(q Query) string {
 	// is threshold relevant?
 	if math.Abs(diffPercent) > q.DiffThreshold {
 		if q.BiggerIsBetter { // higher is better
+			emoji := badEmoji
 			if q.TargetResult > q.BaseResult {
-				return fmt.Sprintf("%s %s", goodEmoji, res)
-			} else {
-				return fmt.Sprintf("%s %s", badEmoji, res)
-			}
-		} else { // lower is better
-			if q.TargetResult < q.BaseResult {
-				return fmt.Sprintf("%s %s", goodEmoji, res)
-			} else {
-				return fmt.Sprintf("%s %s", badEmoji, res)
+				emoji = goodEmoji
 			}
+
+			return fmt.Sprintf("%s %s", emoji, res)
+		}
+
+		// lower is better
+		emoji := badEmoji
+		if q.TargetResult < q.BaseResult {
+			emoji = goodEmoji
 		}
+		return fmt.Sprintf("%s %s", emoji, res)
 	}
 
 	return res

benchmark/internal/cireport/upload.go

@@ -11,14 +11,9 @@ import (
 	"github.com/sirupsen/logrus"
 )
 
-type fs struct {
-}
-
-func NewFsWriter() *fs {
-	return &fs{}
-}
+type FsWriter struct{}
 
-func (rs *fs) WriteFile(dest string, data []byte) (string, error) {
+func (*FsWriter) WriteFile(dest string, data []byte) (string, error) {
 	if _, err := os.Stat(dest); err != nil && !os.IsNotExist(err) {
 		// unkown error
 		return "", err
@@ -35,25 +30,16 @@ func (rs *fs) WriteFile(dest string, data []byte) (string, error) {
 		return "", err
 	}
 
-	// not really useful since we don't have the full path
-	if _, err := os.Stat(dest); err == nil {
-		// file exists
-	} else if os.IsNotExist(err) {
-		// file does not exist
-	} else {
-		// unkown error
-		return "", err
-	}
 	return "file://" + dest, nil
 }
 
-type s3Writer struct {
+type S3Writer struct {
 	bucket *s3go.Bucket
 }
 
-// NewS3Writer creates a s3
+// NewS3Writer creates a s3 writer
 // it assumes AWS environment variables are setup correctly
-func NewS3Writer(bucketName string) (*s3Writer, error) {
+func NewS3Writer(bucketName string) (*S3Writer, error) {
 	if bucketName == "" {
 		return nil, fmt.Errorf("bucket name can't be empty")
 	}
@@ -66,12 +52,12 @@ func NewS3Writer(bucketName string) (*s3Writer, error) {
 	s3 := s3go.New("", keys)
 	b := s3.Bucket(bucketName)
 
-	return &s3Writer{
+	return &S3Writer{
 		bucket: b,
 	}, nil
 }
 
-func (s3Writer *s3Writer) WriteFile(dest string, data []byte) (string, error) {
+func (s3Writer *S3Writer) WriteFile(dest string, data []byte) (string, error) {
 	if dest == "" {
 		return "", fmt.Errorf("filename can't be null")
 	}

benchmark/internal/config/config.go

@@ -1,5 +1,7 @@
 package config
 
+//revive:disable:line-length-limit Most of line length is documentation
+//revive:disable:max-public-structs Config structs
 type Config struct {
 	LoadGen             LoadGen             `skip:"true" mapstructure:",squash"`
 	PromQuery           PromQuery           `skip:"true" mapstructure:",squash"`
@@ -42,7 +44,7 @@ type TableReport struct {
 
 type ImageReport struct {
 	GrafanaAddress string `def:"http://localhost:4050" desc:"address of the grafana instance"`
-	DashboardUid   string `def:"QF9YgRbUbt3BA5Qd" desc:"UUID of the dashboard"`
+	DashboardUID   string `def:"QF9YgRbUbt3BA5Qd" desc:"UUID of the dashboard"`
 	UploadType     string `def:"fs" desc:"where to upload to: s3|fs" mapstructure:"upload-type"`
 	UploadBucket   string `def:"" desc:"bucket name if applicable" mapstructure:"upload-bucket"`
 	UploadDest     string `def:"dashboard-screenshots" desc:"name of the output directory" mapstructure:"upload-dest"`
@@ -62,7 +64,7 @@ type DashboardScreenshot struct {
 	TimeoutSeconds int    `def:"300" desc:"timeout in seconds of each call"`
 
 	GrafanaAddress string `def:"http://localhost:4050" desc:"address of the grafana instance"`
-	DashboardUid   string `def:"QF9YgRbUbt3BA5Qd" desc:"UUID of the dashboard"`
+	DashboardUID   string `def:"QF9YgRbUbt3BA5Qd" desc:"UUID of the dashboard"`
 	Destination    string `def:"fs" desc:"where to upload to: s3|fs"`
 }
 

benchmark/internal/loadgen/loadgen.go

@@ -39,7 +39,7 @@ type GatewayPusher interface {
 }
 type NoopGatewayPusher struct{}
 
-func (n NoopGatewayPusher) Push() error {
+func (NoopGatewayPusher) Push() error {
 	return nil
 }