score-compose
score-compose copied to clipboard
Published
20 hours ago •
score-spec
score-spec
ci: add go-static-check
trafficstars
Add Go Static Check to CI Workflow
Description
This PR adds Staticcheck to the CI pipeline to perform static code analysis on the Go codebase. It helps catch bugs and enforce best practices automatically.
What does this PR do?
It integrates Staticcheck into the CI pipeline, improving code quality and catching potential issues early.
Types of changes
- [ ] Bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to change)
- [x] New chore (expected functionality to be implemented)
Checklist:
- [] My change requires a change to the documentation.
- [ ] I have updated the documentation accordingly.
- [x] I've signed off with an email address that matches the commit author.
@astromechza , what do you think of it? Do you think we should add this to our workflow?
@7h3-3mp7y-m4n personally I'd prefer we go further an add golangci-lint with a config like
version: "2"
linters:
default: standard
enable:
- bodyclose
- contextcheck
- goconst
- gosec
- mnd
- testifylint
I use this in a number of other projects and found it very useful!
@7h3-3mp7y-m4n personally I'd prefer we go further an add golangci-lint with a config like
version: "2" linters: default: standard enable: - bodyclose - contextcheck - goconst - gosec - mnd - testifylintI use this in a number of other projects and found it very useful!
Yeah, makes sense golangci-lint seems like the better call, especially with all the extra checks it brings in. Having everything in one place with a shared config will be super handy. Let’s do it!
I’ll just fix the remaining staticCheck issues first so we can include it all in the same PR
I’m thinking we should remove the go vet step from our ci.yaml, because we can include go vet as a linter in golangci-lint. Since we're centralizing our linting configuration in .golangci.yml , why not add it there as well? What do you think, @astromechza and @mathieu-benoit?
I think this makes sense if it's redundant.
@astromechza, does it work for you?
@7h3-3mp7y-m4n @mathieu-benoit yes, we can remove vet if we're using the new linter.
@7h3-3mp7y-m4n @mathieu-benoit yes, we can remove vet if we're using the new linter.
I'll make the changes :)
here is a little breakdown of linters.
make lint
Running golangci-lint...
golangci-lint run ./...
internal/command/init.go:229:19: Error return value of `f.Close` is not checked (errcheck)
defer f.Close()
^
internal/command/run.go:101:17: Error return value of `src.Close` is not checked (errcheck)
defer src.Close()
^
internal/command/run.go:115:19: Error return value of `ovr.Close` is not checked (errcheck)
defer ovr.Close()
^
internal/command/run.go:273:23: Error return value of `destFile.Close` is not checked (errcheck)
defer destFile.Close()
^
internal/command/run.go:292:19: Error return value of `dest.Close` is not checked (errcheck)
defer dest.Close()
^
internal/command/provisioners.go:79:7: string `json` has 3 occurrences, make it a constant (goconst)
case "json":
^
internal/command/generate.go:377:20: G306: Expect WriteFile permissions to be 0600 or less (gosec)
} else if err := os.WriteFile(v+".temp", raw, 0644); err != nil {
^
internal/command/init.go:225:16: G302: Expect file permissions to be 0600 or less (gosec)
f, err := os.OpenFile(defaultProvisioners, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0644)
^
internal/command/run.go:235:22: G115: integer overflow conversion int -> uint32 (gosec)
Target: uint32(util.DerefOr(pSpec.TargetPort, pSpec.Port)),
^
internal/compose/convert.go:253:19: G304: Potential file inclusion via variable (gosec)
content, err = os.ReadFile(sourcePath)
^
internal/compose/convert.go:291:26: G115: integer overflow conversion int64 -> uint32 (gosec)
fileMode = os.FileMode(newMode)
^
internal/project/project.go:66:12: G301: Expect directory permissions to be 0750 or less (gosec)
if err := os.Mkdir(sd.Path, 0755); err != nil && !errors.Is(err, os.ErrExist) {
^
internal/project/project.go:69:12: G301: Expect directory permissions to be 0750 or less (gosec)
if err := os.Mkdir(sd.State.Extras.MountsDirectory, 0755); err != nil && !errors.Is(err, os.ErrExist) {
^
internal/project/project.go:80:12: G306: Expect WriteFile permissions to be 0600 or less (gosec)
if err := os.WriteFile(filepath.Join(sd.Path, StateFileName+".temp"), out.Bytes(), 0755); err != nil {
^
internal/project/project.go:91:18: G304: Potential file inclusion via variable (gosec)
content, err := os.ReadFile(filepath.Join(d, StateFileName))
^
internal/provisioners/cmdprov/commandprov.go:134:9: G204: Subprocess launched with variable (gosec)
cmd := exec.CommandContext(ctx, bin, p.Args...)
^
internal/provisioners/core.go:212:14: G301: Expect directory permissions to be 0750 or less (gosec)
if err := os.MkdirAll(dst, 0755); err != nil && !errors.Is(err, os.ErrExist) {
^
internal/provisioners/core.go:234:14: G306: Expect WriteFile permissions to be 0600 or less (gosec)
if err := os.WriteFile(dst, []byte(*b), 0644); err != nil {
^
internal/provisioners/loader/load.go:87:16: G304: Potential file inclusion via variable (gosec)
raw, err := os.ReadFile(filepath.Join(path, item.Name()))
^
internal/provisioners/loader/load.go:114:99: G115: integer overflow conversion int64 -> uint64 (gosec)
timePrefix := base64.RawURLEncoding.EncodeToString(binary.BigEndian.AppendUint64([]byte{}, uint64(math.MaxInt64-time.Now().UnixNano())))
^
internal/command/generate.go:172:46: Magic number: 2, in <argument> detected (mnd)
parts := strings.SplitN(buildFlag, "=", 2)
^
internal/command/generate.go:173:23: Magic number: 2, in <condition> detected (mnd)
if len(parts) != 2 {
^
internal/command/generate.go:296:22: Magic number: 2, in <condition> detected (mnd)
if len(parts) <= 2 {
^
internal/compose/convert.go:217:54: Magic number: 5, in <argument> detected (mnd)
Interval: util.Ref(compose.Duration(time.Second * 5)),
^
internal/compose/convert.go:218:54: Magic number: 5, in <argument> detected (mnd)
Timeout: util.Ref(compose.Duration(time.Second * 5)),
^
internal/compose/convert.go:278:27: Magic number: 0644, in <argument> detected (mnd)
fileMode := os.FileMode(0644)
^
internal/compose/convert.go:283:24: Magic number: 0777, in <condition> detected (mnd)
} else if newMode > 0777 {
^
internal/compose/convert.go:285:30: Magic number: 0400, in <condition> detected (mnd)
} else if newMode&0400 != 0400 {
^
internal/compose/convert.go:287:30: Magic number: 0600, in <condition> detected (mnd)
} else if newMode&0600 != 0600 {
^
internal/compose/convert.go:288:25: Magic number: 0600, in <operation> detected (mnd)
newMode = newMode | 0600
^
internal/compose/write.go:27:16: Magic number: 2, in <argument> detected (mnd)
enc.SetIndent(2)
^
internal/project/project.go:74:16: Magic number: 2, in <argument> detected (mnd)
enc.SetIndent(2)
^
internal/provisioners/loader/load.go:118:40: Magic number: 0600, in <argument> detected (mnd)
if err := os.WriteFile(tmpPath, data, 0600); err != nil {
^
internal/version/version.go:51:16: Magic number: 2, in <condition> detected (mnd)
if len(cpm) > 2 {
^
internal/version/version.go:53:17: Magic number: 3, in <condition> detected (mnd)
if len(cpm) > 3 {
^
internal/logging/logging.go:15:1: package-comments: should have a package comment (revive)
package logging
^
internal/logging/logging.go:25:6: exported: exported type SimpleHandler should have comment or be unexported (revive)
type SimpleHandler struct {
^
internal/logging/logging.go:32:33: unused-parameter: parameter 'ctx' seems to be unused, consider removing or renaming it as _ (revive)
func (h *SimpleHandler) Enabled(ctx context.Context, level slog.Level) bool {
^
internal/logging/logging.go:36:32: unused-parameter: parameter 'ctx' seems to be unused, consider removing or renaming it as _ (revive)
func (h *SimpleHandler) Handle(ctx context.Context, record slog.Record) error {
^
internal/logging/logging.go:43:35: unused-parameter: parameter 'attrs' seems to be unused, consider removing or renaming it as _ (revive)
func (h *SimpleHandler) WithAttrs(attrs []slog.Attr) slog.Handler {
^
internal/logging/logging.go:48:35: unused-parameter: parameter 'name' seems to be unused, consider removing or renaming it as _ (revive)
func (h *SimpleHandler) WithGroup(name string) slog.Handler {
^
internal/patching/patching.go:36:6: exported: exported type PatchOperation should have comment or be unexported (revive)
type PatchOperation struct {
^
internal/project/project.go:15:1: package-comments: should have a package comment (revive)
package project
^
internal/project/project.go:30:2: exported: exported const DefaultRelativeStateDirectory should have comment (or a comment on this block) or be unexported (revive)
DefaultRelativeStateDirectory = ".score-compose"
^
internal/project/project.go:39:6: exported: exported type StateExtras should have comment or be unexported (revive)
type StateExtras struct {
^
internal/project/project.go:45:6: exported: exported type WorkloadExtras should have comment or be unexported (revive)
type WorkloadExtras struct {
^
internal/provisioners/cmdprov/commandprov.go:36:6: exported: exported type Provisioner should have comment or be unexported (revive)
type Provisioner struct {
^
internal/provisioners/cmdprov/commandprov.go:37:2: var-naming: struct field ProvisionerUri should be ProvisionerURI (revive)
ProvisionerUri string `yaml:"uri"`
^
internal/provisioners/cmdprov/commandprov.go:40:2: var-naming: struct field ResId should be ResID (revive)
ResId *string `yaml:"id,omitempty"`
^
internal/provisioners/cmdprov/commandprov.go:47:1: exported: exported method Provisioner.Description should have comment or be unexported (revive)
func (p *Provisioner) Description() string {
^
internal/provisioners/cmdprov/commandprov.go:55:1: exported: exported method Provisioner.Class should have comment or be unexported (revive)
func (p *Provisioner) Class() string {
^
internal/provisioners/cmdprov/commandprov.go:62:1: exported: exported method Provisioner.Type should have comment or be unexported (revive)
func (p *Provisioner) Type() string {
^
internal/provisioners/cmdprov/commandprov.go:66:1: exported: exported method Provisioner.Params should have comment or be unexported (revive)
func (p *Provisioner) Params() []string {
^
internal/provisioners/cmdprov/commandprov.go:70:1: exported: exported method Provisioner.Outputs should have comment or be unexported (revive)
func (p *Provisioner) Outputs() []string {
^
internal/provisioners/cmdprov/commandprov.go:122:1: exported: exported method Provisioner.Provision should have comment or be unexported (revive)
func (p *Provisioner) Provision(ctx context.Context, input *provisioners.Input) (*provisioners.ProvisionOutput, error) {
^
internal/provisioners/cmdprov/commandprov.go:154:1: exported: exported function Parse should have comment or be unexported (revive)
func Parse(raw map[string]interface{}) (*Provisioner, error) {
^
internal/provisioners/core.go:41:2: var-naming: struct field ResourceUid should be ResourceUID (revive)
ResourceUid string `json:"resource_uid"`
^
internal/provisioners/core.go:44:2: var-naming: struct field ResourceId should be ResourceID (revive)
ResourceId string `json:"resource_id"`
^
internal/provisioners/core.go:67:6: exported: exported type ServicePort should have comment or be unexported (revive)
type ServicePort struct {
^
internal/provisioners/core.go:86:2: var-naming: struct field ProvisionerUri should be ProvisionerURI (revive)
ProvisionerUri string `json:"-"`
^
internal/provisioners/core.go:100:6: exported: exported type Provisioner should have comment or be unexported (revive)
type Provisioner interface {
^
internal/provisioners/core.go:102:8: var-naming: interface method parameter resUid should be resUID (revive)
Match(resUid framework.ResourceUid) bool
^
internal/provisioners/core.go:113:2: var-naming: struct field matchUid should be matchUID (revive)
matchUid framework.ResourceUid
^
internal/provisioners/core.go:126:32: var-naming: method Uri should be URI (revive)
func (e *ephemeralProvisioner) Uri() string {
^
internal/provisioners/core.go:130:38: var-naming: method parameter resUid should be resUID (revive)
func (e *ephemeralProvisioner) Match(resUid framework.ResourceUid) bool {
^
internal/provisioners/core.go:154:42: var-naming: func parameter matchUid should be matchUID (revive)
func NewEphemeralProvisioner(uri string, matchUid framework.ResourceUid, inner func(ctx context.Context, input *Input) (*ProvisionOutput, error)) Provisioner {
^
internal/provisioners/core.go:160:73: var-naming: method parameter resUid should be resUID (revive)
func (po *ProvisionOutput) ApplyToStateAndProject(state *project.State, resUid framework.ResourceUid, project *compose.Project) (*project.State, error) {
^
internal/provisioners/core.go:300:1: exported: exported function ProvisionResources should have comment or be unexported (revive)
func ProvisionResources(ctx context.Context, state *project.State, provisioners []Provisioner, composeProject *compose.Project) (*project.State, error) {
^
internal/provisioners/core.go:311:9: var-naming: range var resUid should be resUID (revive)
for _, resUid := range orderedResources {
^
internal/provisioners/envprov/envprov.go:41:23: var-naming: method Uri should be URI (revive)
func (e *Provisioner) Uri() string {
^
internal/provisioners/envprov/envprov.go:45:29: var-naming: method parameter resUid should be resUID (revive)
func (e *Provisioner) Match(resUid framework.ResourceUid) bool {
^
internal/provisioners/envprov/envprov.go:49:33: unused-parameter: parameter 'ctx' seems to be unused, consider removing or renaming it as _ (revive)
func (e *Provisioner) Provision(ctx context.Context, input *provisioners.Input) (*provisioners.ProvisionOutput, error) {
^
internal/provisioners/envprov/envprov.go:56:1: exported: exported method Provisioner.Accessed should have comment or be unexported (revive)
func (e *Provisioner) Accessed() map[string]string {
^
internal/provisioners/envprov/envprov.go:76:1: exported: exported method Provisioner.LookupOutput should have comment or be unexported (revive)
func (e *Provisioner) LookupOutput(keys ...string) (interface{}, error) {
^
internal/provisioners/envprov/envprov.go:118:33: var-naming: method Uri should be URI (revive)
func (e *envVarResourceTracker) Uri() string {
^
internal/provisioners/envprov/envprov.go:156:1: receiver-naming: receiver name p should be consistent with previous receiver name e for Provisioner (revive)
func (p *Provisioner) Class() string {
return p.Class()
}
internal/provisioners/envprov/envprov.go:160:1: receiver-naming: receiver name p should be consistent with previous receiver name e for Provisioner (revive)
func (p *Provisioner) Type() string {
return p.Type()
}
internal/provisioners/envprov/envprov.go:164:1: receiver-naming: receiver name p should be consistent with previous receiver name e for Provisioner (revive)
func (p *Provisioner) Outputs() []string {
return nil
}
internal/util/maps.go:15:1: package-comments: should have a package comment (revive)
package util
^
internal/util/ref.go:17:1: exported: exported function Ref should have comment or be unexported (revive)
func Ref[k any](input k) *k {
^
internal/util/ref.go:21:1: exported: exported function DerefOr should have comment or be unexported (revive)
func DerefOr[k any](input *k, def k) k {
^
internal/version/version.go:1:1: package-comments: package comment should be of the form "Package version ..." (revive)
/*
Apache Score
Copyright 2022 The Apache Software Foundation
This product includes software developed at
The Apache Software Foundation (http://www.apache.org/).
*/
internal/version/version.go:18:22: var-declaration: should omit type string from declaration of var Version; it will be inferred from the right-hand side (revive)
Version string = "0.0.0"
^
internal/version/version.go:60:1: exported: exported function AssertVersion should have comment or be unexported (revive)
func AssertVersion(constraint string, current string) error {
^
internal/version/version.go:65:9: indent-error-flow: if block ends with a return statement, so drop this else and outdent its block (move short variable declaration to its own line if necessary) (revive)
} else {
op := m[1]
compareI, err := semverToI(m[0][len(op):])
if err != nil {
return fmt.Errorf("failed to parse constraint: %w", err)
}
match := false
switch op {
case ">":
match = currentI > compareI
case ">=":
match = currentI >= compareI
case "=":
match = currentI == compareI
}
if !match {
return fmt.Errorf("current version %s does not match requested constraint %s", current, constraint)
}
return nil
}
internal/logging/logging.go:39:12: QF1012: Use fmt.Fprintf(...) instead of Write([]byte(fmt.Sprintf(...))) (staticcheck)
_, err := h.Writer.Write([]byte(fmt.Sprintf("%s: %s\n", record.Level.String(), record.Message)))
^
internal/provisioners/envprov/envprov.go:157:9: SA5007: infinite recursive call (staticcheck)
return p.Class()
^
internal/provisioners/envprov/envprov.go:161:9: SA5007: infinite recursive call (staticcheck)
return p.Type()
^
internal/provisioners/envprov/envprov.go:181:9: SA5007: infinite recursive call (staticcheck)
return p.Description()
^
89 issues:
* errcheck: 5
* goconst: 1
* gosec: 14
* mnd: 15
* revive: 50
* staticcheck: 4
make: *** [lint] Error 1
revive is a good linter, but it has strict rules and might discourage new contributors. It can also create noise, so let's not use it for now.