Skip to content

fixes: enable golanci-lint in CI, fix complaints. #232

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Oct 16, 2024
Merged

fixes: enable golanci-lint in CI, fix complaints. #232

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b414729
Makefile: add [golang]ci-lint target.
klihub Oct 15, 2024
78c6041
pkg/cdi: address complaints by golangci-lint.
klihub Oct 15, 2024
9927475
.github: add golangci-lint workflow.
klihub Oct 15, 2024
84ea77f
pkg/cdi: document possible implicit cache refresh.
klihub Oct 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions .github/workflows/golangci-lint.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
name: CI
on:
push:
branches:
- main
pull_request:
branches:
- main

permissions:
contents: read
# Optional: allow read access to pull request. Use with `only-new-issues` option.
# pull-requests: read

jobs:
golangci:
name: lint
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4
- uses: actions/setup-go@v5
with:
go-version: stable
- name: golangci-lint
uses: golangci/golangci-lint-action@v6
with:
version: v1.61.0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to specify the version, or should we just use the "default"?

Copy link
Contributor Author

@klihub klihub Oct 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what would make the most sense here. I simply took the provided sample workflow as a basis for this, then bumped the version to the latest release, which I then also used locally for linting.

Looking at the action implementation, it is stated that we could use an explicit 'latest' here. But TBH, I'm not too keen on a setup where we are not in control so without us changing anything, things already merged might start getting flagged in new PRs as linting errors.

Also looking at other projects, most of them seem to lock the version explicitly to one of their choice, which they then keep gradually updating. If we really wanted to go with an uncontrolled rolling version, then we'd need to add a nightly run, to catch extra-repo (IOW golangci-lint) changes breaking things for us...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @klihub here. To have a reproducible builds (tests in this case) we need to lock lint version.

6 changes: 5 additions & 1 deletion Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ GO_TEST := $(GO_CMD) test -race -v -cover
GO_LINT := golint -set_exit_status
GO_FMT := gofmt
GO_VET := $(GO_CMD) vet
CI_LINT := golangci-lint

CDI_PKG := $(shell grep ^module go.mod | sed 's/^module *//g')

Expand Down Expand Up @@ -43,7 +44,7 @@ test: test-gopkgs test-schema
# validation targets
#

pre-pr-checks pr-checks: test fmt lint vet
pre-pr-checks pr-checks: test fmt lint vet ci-lint

fmt format:
$(Q)$(GO_FMT) -s -d -w -e .
Expand All @@ -52,6 +53,9 @@ lint:
$(Q)$(GO_LINT) -set_exit_status ./...
vet:
$(Q)$(GO_VET) ./...
golangci-lint ci-lint:
$(Q)$(CI_LINT) run


#
# build targets
Expand Down
35 changes: 21 additions & 14 deletions pkg/cdi/cache.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ func (c *Cache) configure(options ...Option) {
c.watch.setup(c.specDirs, c.dirErrors)
c.watch.start(&c.Mutex, c.refresh, c.dirErrors)
}
c.refresh()
_ = c.refresh() // we record but ignore errors
}

// Refresh rescans the CDI Spec directories and refreshes the Cache.
Expand Down Expand Up @@ -222,7 +222,8 @@ func (c *Cache) refreshIfRequired(force bool) (bool, error) {

// InjectDevices injects the given qualified devices to an OCI Spec. It
// returns any unresolvable devices and an error if injection fails for
// any of the devices.
// any of the devices. Might trigger a cache refresh, in which case any
// errors encountered can be obtained using GetErrors().
func (c *Cache) InjectDevices(ociSpec *oci.Spec, devices ...string) ([]string, error) {
var unresolved []string

Expand All @@ -233,7 +234,7 @@ func (c *Cache) InjectDevices(ociSpec *oci.Spec, devices ...string) ([]string, e
c.Lock()
defer c.Unlock()

c.refreshIfRequired(false)
_, _ = c.refreshIfRequired(false) // we record but ignore errors

edits := &ContainerEdits{}
specs := map[*Spec]struct{}{}
Expand Down Expand Up @@ -335,24 +336,27 @@ func (c *Cache) RemoveSpec(name string) error {
return err
}

// GetDevice returns the cached device for the given qualified name.
// GetDevice returns the cached device for the given qualified name. Might trigger
// a cache refresh, in which case any errors encountered can be obtained using
// GetErrors().
func (c *Cache) GetDevice(device string) *Device {
c.Lock()
defer c.Unlock()

c.refreshIfRequired(false)
_, _ = c.refreshIfRequired(false) // we record but ignore errors

return c.devices[device]
}

// ListDevices lists all cached devices by qualified name.
// ListDevices lists all cached devices by qualified name. Might trigger a cache
// refresh, in which case any errors encountered can be obtained using GetErrors().
func (c *Cache) ListDevices() []string {
var devices []string

c.Lock()
defer c.Unlock()

c.refreshIfRequired(false)
_, _ = c.refreshIfRequired(false) // we record but ignore errors

for name := range c.devices {
devices = append(devices, name)
Expand All @@ -362,14 +366,15 @@ func (c *Cache) ListDevices() []string {
return devices
}

// ListVendors lists all vendors known to the cache.
// ListVendors lists all vendors known to the cache. Might trigger a cache refresh,
// in which case any errors encountered can be obtained using GetErrors().
func (c *Cache) ListVendors() []string {
var vendors []string

c.Lock()
defer c.Unlock()

c.refreshIfRequired(false)
_, _ = c.refreshIfRequired(false) // we record but ignore errors

for vendor := range c.specs {
vendors = append(vendors, vendor)
Expand All @@ -379,7 +384,8 @@ func (c *Cache) ListVendors() []string {
return vendors
}

// ListClasses lists all device classes known to the cache.
// ListClasses lists all device classes known to the cache. Might trigger a cache
// refresh, in which case any errors encountered can be obtained using GetErrors().
func (c *Cache) ListClasses() []string {
var (
cmap = map[string]struct{}{}
Expand All @@ -389,7 +395,7 @@ func (c *Cache) ListClasses() []string {
c.Lock()
defer c.Unlock()

c.refreshIfRequired(false)
_, _ = c.refreshIfRequired(false) // we record but ignore errors

for _, specs := range c.specs {
for _, spec := range specs {
Expand All @@ -404,12 +410,13 @@ func (c *Cache) ListClasses() []string {
return classes
}

// GetVendorSpecs returns all specs for the given vendor.
// GetVendorSpecs returns all specs for the given vendor. Might trigger a cache
// refresh, in which case any errors encountered can be obtained using GetErrors().
func (c *Cache) GetVendorSpecs(vendor string) []*Spec {
c.Lock()
defer c.Unlock()

c.refreshIfRequired(false)
_, _ = c.refreshIfRequired(false) // we record but ignore errors

return c.specs[vendor]
}
Expand Down Expand Up @@ -544,7 +551,7 @@ func (w *watch) watch(fsw *fsnotify.Watcher, m *sync.Mutex, refresh func() error
} else {
w.update(dirErrors)
}
refresh()
_ = refresh()
m.Unlock()

case _, ok := <-watch.Errors:
Expand Down
9 changes: 6 additions & 3 deletions pkg/cdi/default-cache_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,8 @@ devices:
filepath.Join(dir, "run"),
),
}
Configure(opts...)
err = Configure(opts...)
require.NoError(t, err, "unexpected Configure() error")

devices := cache.ListDevices()
if len(tc.devices) == 0 {
Expand Down Expand Up @@ -261,7 +262,8 @@ devices:
filepath.Join(dir, "run"),
),
}
Configure(opts...)
err = Configure(opts...)
require.NoError(t, err, "unexpected Configure() error")
} else {
err = updateSpecDirs(dir, update.etc, update.run)
if err != nil {
Expand Down Expand Up @@ -395,12 +397,13 @@ devices:
return
}

Configure(
err = Configure(
WithSpecDirs(
filepath.Join(dir, "etc"),
filepath.Join(dir, "run"),
),
)
require.NoError(t, err, "unexpected Configure() error")

unresolved, err := InjectDevices(tc.ociSpec, tc.devices...)
if len(tc.unresolved) != 0 {
Expand Down