Skip to content
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

Jump to bottom

Update static analysis #12011

Closed
markylaing opened this issue Jul 13, 2023 · 10 comments
Closed

Update static analysis #12011

markylaing opened this issue Jul 13, 2023 · 10 comments
Assignees
@markylaing
Labels
Easy Good for new contributors

Comments

@markylaing
Copy link
Contributor

markylaing commented Jul 13, 2023 •
edited
Loading

There are a few tasks that can be carried out to improve our static analysis.

First, the golangci-lint installation script will automatically select the most recent version if no version is specified. This was implemented for microcloud here. We should also do this for LXD to make sure our linters stay up-to-date.

Second, the errcheck linter defaults to skip errors that have been explicitly ignored. We have discussed this here and think it would be beneficial to disallow unchecked errors without a //nolint:errcheck directive. This may seem tedious but it will at least prompt us to comment with a reason as to why an error is ignored. Most should be at least logger (see the discussion). PRs for this change should be package-by-package because there are so many.

  • Auto-install latest golangci-lint during github workflow.
  • Fix errcheck: github.com/canonical/lxd/client
  • Fix errcheck: github.com/canonical/lxd/fuidshift
  • Fix errcheck: github.com/canonical/lxd/lxc
  • Fix errcheck: github.com/canonical/lxd/lxc/config
  • Fix errcheck: github.com/canonical/lxd/lxc-to-lxd
  • Fix errcheck: github.com/canonical/lxd/lxd
  • Fix errcheck: github.com/canonical/lxd/lxd/acme
  • Fix errcheck: github.com/canonical/lxd/lxd/apparmor
  • Fix errcheck: github.com/canonical/lxd/lxd/archive
  • Fix errcheck: github.com/canonical/lxd/lxd/auth/candid
  • Fix errcheck: github.com/canonical/lxd/lxd/auth/oidc
  • Fix errcheck: github.com/canonical/lxd/lxd/backup
  • Fix errcheck: github.com/canonical/lxd/lxd/backup/config
  • Fix errcheck: github.com/canonical/lxd/lxd/bgp
  • Fix errcheck: github.com/canonical/lxd/lxd/cgroup
  • Fix errcheck: github.com/canonical/lxd/lxd/cluster
  • Fix errcheck: github.com/canonical/lxd/lxd/cluster/config
  • Fix errcheck: github.com/canonical/lxd/lxd/cluster/request
  • Fix errcheck: github.com/canonical/lxd/lxd/config
  • Fix errcheck: github.com/canonical/lxd/lxd/config/generate
  • Fix errcheck: github.com/canonical/lxd/lxd/daemon
  • Fix errcheck: github.com/canonical/lxd/lxd/db
  • Fix errcheck: github.com/canonical/lxd/lxd/db/cluster
  • Fix errcheck: github.com/canonical/lxd/lxd/db/generate
  • Fix errcheck: github.com/canonical/lxd/lxd/db/generate/db
  • Fix errcheck: github.com/canonical/lxd/lxd/db/generate/file
  • Fix errcheck: github.com/canonical/lxd/lxd/db/generate/lex
  • Fix errcheck: github.com/canonical/lxd/lxd/db/node
  • Fix errcheck: github.com/canonical/lxd/lxd/db/operationtype
  • Fix errcheck: github.com/canonical/lxd/lxd/db/query
  • Fix errcheck: github.com/canonical/lxd/lxd/db/schema
  • Fix errcheck: github.com/canonical/lxd/lxd/db/warningtype
  • Fix errcheck: github.com/canonical/lxd/lxd/device
  • Fix errcheck: github.com/canonical/lxd/lxd/device/config
  • Fix errcheck: github.com/canonical/lxd/lxd/device/nictype
  • Fix errcheck: github.com/canonical/lxd/lxd/device/pci
  • Fix errcheck: github.com/canonical/lxd/lxd/dns
  • Fix errcheck: github.com/canonical/lxd/lxd/dnsmasq
  • Fix errcheck: github.com/canonical/lxd/lxd/dnsmasq/dhcpalloc
  • Fix errcheck: github.com/canonical/lxd/lxd/endpoints
  • Fix errcheck: github.com/canonical/lxd/lxd/endpoints/listeners
  • Fix errcheck: github.com/canonical/lxd/lxd/events
  • Fix errcheck: github.com/canonical/lxd/lxd/firewall
  • Fix errcheck: github.com/canonical/lxd/lxd/firewall/drivers
  • Fix errcheck: github.com/canonical/lxd/lxd/fsmonitor
  • Fix errcheck: github.com/canonical/lxd/lxd/fsmonitor/drivers
  • Fix errcheck: github.com/canonical/lxd/lxd/include
  • Fix errcheck: github.com/canonical/lxd/lxd/instance
  • Fix errcheck: github.com/canonical/lxd/lxd/instance/drivers
  • Fix errcheck: github.com/canonical/lxd/lxd/instance/drivers/qmp
  • Fix errcheck: github.com/canonical/lxd/lxd/instance/instancetype
  • Fix errcheck: github.com/canonical/lxd/lxd/instance/operationlock
  • Fix errcheck: github.com/canonical/lxd/lxd/ip
  • Fix errcheck: github.com/canonical/lxd/lxd/lifecycle
  • Fix errcheck: github.com/canonical/lxd/lxd/locking
  • Fix errcheck: github.com/canonical/lxd/lxd/loki
  • Fix errcheck: github.com/canonical/lxd/lxd/maas
  • Fix errcheck: github.com/canonical/lxd/lxd/metrics
  • Fix errcheck: github.com/canonical/lxd/lxd/migration
  • Fix errcheck: github.com/canonical/lxd/lxd/network
  • Fix errcheck: github.com/canonical/lxd/lxd/network/acl
  • Fix errcheck: github.com/canonical/lxd/lxd/network/openvswitch
  • Fix errcheck: github.com/canonical/lxd/lxd/network/zone
  • Fix errcheck: github.com/canonical/lxd/lxd/node
  • Fix errcheck: github.com/canonical/lxd/lxd/operations
  • Fix errcheck: github.com/canonical/lxd/lxd/project
  • Fix errcheck: github.com/canonical/lxd/lxd/rbac
  • Fix errcheck: github.com/canonical/lxd/lxd/refcount
  • Fix errcheck: github.com/canonical/lxd/lxd/request
  • Fix errcheck: github.com/canonical/lxd/lxd/resources
  • Fix errcheck: github.com/canonical/lxd/lxd/response
  • Fix errcheck: github.com/canonical/lxd/lxd/revert
  • Fix errcheck: github.com/canonical/lxd/lxd/rsync
  • Fix errcheck: github.com/canonical/lxd/lxd/scriptlet
  • Fix errcheck: github.com/canonical/lxd/lxd/scriptlet/load
  • Fix errcheck: github.com/canonical/lxd/lxd/seccomp
  • Fix errcheck: github.com/canonical/lxd/lxd/state
  • Fix errcheck: github.com/canonical/lxd/lxd/storage
  • Fix errcheck: github.com/canonical/lxd/lxd/storage/drivers
  • Fix errcheck: github.com/canonical/lxd/lxd/storage/filesystem
  • Fix errcheck: github.com/canonical/lxd/lxd/storage/memorypipe
  • Fix errcheck: github.com/canonical/lxd/lxd/storage/quota
  • Fix errcheck: github.com/canonical/lxd/lxd/storage/s3
  • Fix errcheck: github.com/canonical/lxd/lxd/storage/s3/miniod
  • Fix errcheck: github.com/canonical/lxd/lxd/sys
  • Fix errcheck: github.com/canonical/lxd/lxd/task
  • Fix errcheck: github.com/canonical/lxd/lxd/template
  • Fix errcheck: github.com/canonical/lxd/lxd/ucred
  • Fix errcheck: github.com/canonical/lxd/lxd/util
  • Fix errcheck: github.com/canonical/lxd/lxd/vsock
  • Fix errcheck: github.com/canonical/lxd/lxd/warnings
  • Fix errcheck: github.com/canonical/lxd/lxd-agent
  • Fix errcheck: github.com/canonical/lxd/lxd-agent/api
  • Fix errcheck: github.com/canonical/lxd/lxd-benchmark
  • Fix errcheck: github.com/canonical/lxd/lxd-benchmark/benchmark
  • Fix errcheck: github.com/canonical/lxd/lxd-migrate
  • Fix errcheck: github.com/canonical/lxd/lxd-user
  • Fix errcheck: github.com/canonical/lxd/shared
  • Fix errcheck: github.com/canonical/lxd/shared/api
  • Fix errcheck: github.com/canonical/lxd/shared/api/scriptlet
  • Fix errcheck: github.com/canonical/lxd/shared/cancel
  • Fix errcheck: github.com/canonical/lxd/shared/cmd
  • Fix errcheck: github.com/canonical/lxd/shared/dnsutil
  • Fix errcheck: github.com/canonical/lxd/shared/eagain
  • Fix errcheck: github.com/canonical/lxd/shared/filter
  • Fix errcheck: github.com/canonical/lxd/shared/i18n
  • Fix errcheck: github.com/canonical/lxd/shared/idmap
  • Fix errcheck: github.com/canonical/lxd/shared/instancewriter
  • Fix errcheck: github.com/canonical/lxd/shared/ioprogress
  • Fix errcheck: github.com/canonical/lxd/shared/linux
  • Fix errcheck: github.com/canonical/lxd/shared/logger
  • Fix errcheck: github.com/canonical/lxd/shared/netutils
  • Fix errcheck: github.com/canonical/lxd/shared/osarch
  • Fix errcheck: github.com/canonical/lxd/shared/simplestreams
  • Fix errcheck: github.com/canonical/lxd/shared/subprocess
  • Fix errcheck: github.com/canonical/lxd/shared/tcp
  • Fix errcheck: github.com/canonical/lxd/shared/termios
  • Fix errcheck: github.com/canonical/lxd/shared/units
  • Fix errcheck: github.com/canonical/lxd/shared/usbid
  • Fix errcheck: github.com/canonical/lxd/shared/validate
  • Fix errcheck: github.com/canonical/lxd/shared/version
  • Fix errcheck: github.com/canonical/lxd/shared/ws
  • Fix errcheck: github.com/canonical/lxd/test/devlxd-client
  • Fix errcheck: github.com/canonical/lxd/test/macaroon-identity
  • Fix errcheck: github.com/canonical/lxd/test/syscall/sysinfo
@markylaing markylaing self-assigned this Jul 13, 2023
@tomponline
Copy link
Member

@markylaing can we use https://github.com/golangci/golangci-lint-action?

@markylaing
Copy link
Contributor Author

@markylaing can we use https://github.com/golangci/golangci-lint-action?

I think we decided not to because the action runs make static-analysis which includes other linters.

@tomponline
Copy link
Member

OK cool.

Their page says

IMPORTANT: It's highly recommended installing a specific version of golangci-lint available on the releases page.

https://golangci-lint.run/usage/install/#ci-installation

So I'm not sure about always installing the latest version though.

@monstermunchkin
Copy link
Contributor

@tomponline if not always using the latest, we should still update to v1.53.3 as our currently used one is somewhat outdated.

@markylaing
Copy link
Contributor Author

OK cool.

Their page says

IMPORTANT: It's highly recommended installing a specific version of golangci-lint available on the releases page.

https://golangci-lint.run/usage/install/#ci-installation

So I'm not sure about always installing the latest version though.

I'll leave that up to you but I think it's probably best to stay up-to-date automatically and then pin it if we have any issues.

@tomponline
Copy link
Member

I agree we should update certainly.

It just seems odd they don't recommend auto updating.

@markylaing
Copy link
Contributor Author

markylaing commented Jul 13, 2023 •
edited
Loading

I guess they're saying don't absolutely rely on the latest patch, but it's fairly trivial to pin to an older version while they fix things (for us at least) so 🤷

@tomponline
Copy link
Member

Well lets try the auto update, but if it interrupts our workflow by breaking and requiring manual pinning we can just go back to pinning again.

@tomponline
Copy link
Member

This is why they probably recommend pinning:

https://golangci-lint.run/usage/install/#versioning-policy

@tomponline tomponline added the Easy Good for new contributors label Jul 14, 2023
@markylaing markylaing mentioned this issue Jul 14, 2023
Merged
@tomponline tomponline added this to the lxd-5.17 milestone Jul 31, 2023
@tomponline tomponline modified the milestones: lxd-5.17, lxd-5.18 Aug 21, 2023
@tomponline tomponline modified the milestones: lxd-5.18, lxd-5.19 Sep 20, 2023
@tomponline tomponline removed this from the lxd-5.19 milestone Oct 10, 2023
@markylaing markylaing mentioned this issue Oct 10, 2023
Merged
@markylaing
Copy link
Contributor Author

I made a start on this a while back for the client package. I think it's an awful lot of work for very little pay off. Most of the work is in deciding what to do with the ignored errors. E.g. if logging, then at what verbosity? Does the called function ever return an error? Might this change in the future?

We could take a blanket decision to log all ignored errors at the debug level so that they are at least handled in some way, but this is still a lower priority than any other issue or work item.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@markylaing markylaing

Labels
Easy Good for new contributors
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@monstermunchkin @tomponline @markylaing