-
Notifications
You must be signed in to change notification settings - Fork 510
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.
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
Errcheck linter fixes #12614
Errcheck linter fixes #12614
Conversation
The following adds an experimental error checking that attempts to add more linters to the juju code base without having to do all the work in one PR. For instance the current PR only fixes errors in production code and not in test code.
if [ "${VER}" != "1.35" ]; then | ||
(>&2 echo -e "\\nError: golangci-lint version does not match 1.35") | ||
if [ "${VER}" != "1.36" ]; then | ||
(>&2 echo -e "\\nError: golangci-lint version does not match 1.36. Please upgrade/downgrade to the right version.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
The call was not needed, there error wasn't recorded and doesn't do what you expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice. Seems good to me, and caught a couple of real bugs. Does make defer file.Close()
type of things a bit more annoying (do I log or ignore here?), but the trade-off seems worth it.
Possible we could make some helpers for these situations? |
Looks good Simon |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The shear volume of black-holed errors is concerning, particularly ones associated with critical path things like pub-sub and worker start-up.
We've tightened it up some though, and this allows us to ensure that we will not ignore any new errors inadvertently, so it's a win.
|
Looks like the merge bot fails when we make changes in the .github folder. Fine to override for now, but we definitely need to investigate if we start having trouble elsewhere. |
#12657 Merge 2.9 with these PRs: #12566 Make `coreos/go-systemd' buildable on windows #12594 [Charmhub] - `Juju info` updates based on upcoming api changes. #12596 Pebble support with *-workload-ready hook. #12602 Static analysis checks #12604 [2.9] Do not attempt to set proxy settings for windows/centos workloads #12605 [2.9] Use positional arguments for azure clients #12607 Clean up model migration code #12608 Allow bundle to update existing offers #12609 EC2 fix panic #12614 Errcheck linter fixes #12615 Allow bundle to update existing offers #12617 Fixes permission for model operator. #12619 Add set-series integration test #12620 Fix asserts used when finalising an action #12621 Relocates DNS and resolve.conf parsing from network to core/network #12622 Move NetworkConfigSource from api/common to core/network #12623 Indirection for network config sourced addresses #12627 Ensure relation units #12629 Record why a cloud credential gets marked invalid and expose via show-model, status #12630 Fix double upgraded #12633 Update how channel is created in ApplicationsInfo. #12634 Fix uniter loop when a relation is removed out of bandge case. #12637 Improve maas cli output handling in acceptance tests #12638 Bundle export with channel #12641 Bundle diff output with charm url names #12643 Expose charm channel in juju status #12644 Fix service type translation for k8s applications #12647 Fix acceptance test bug in get_uptime #12650 Ensure we normalize the charm url in bundle handler #12651 Use python 3.6 compat fix for handling maas cli output #12652 Use python 3.6 compatible way to capture subprocess stdout/err output #12653 missing container status of creating #12654 Move 2.8.10 upgrade step to 2.8.9 ``` # Conflicts: # api/backups/restore.go # apiserver/facades/client/backups/restore.go # caas/kubernetes/provider/operator.go # cloudconfig/podcfg/image.go # cmd/containeragent/main_nix.go # cmd/containeragent/main_test.go # cmd/containeragent/unit/agent.go # cmd/containeragent/unit/agent_test.go # cmd/juju/action/run.go # cmd/juju/action/showoutput.go # cmd/juju/commands/exec.go # cmd/juju/commands/exec_test.go # go.mod # go.sum # juju/testing/conn.go # state/action_test.go # state/applicationoffers.go # state/backups/files.go # state/backups/restore_test.go # state/upgrades.go # worker/machineactions/worker.go # worker/restorewatcher/manifold.go # worker/restorewatcher/worker.go # worker/uniter/runner/jujuc/mocks/context_relation_mock.go # worker/uniter/util_test.go ``` ## QA steps See PRs
Follow the fallout from #12609 it became apparent that we could employ linters
to help guard and solve against panics at runtime. The problem is that in order to
use the linter we need to enable
errcheck
.The
errcheck
didn't currently run, so the following effort is to make jujuerrcheck
friendly. The next stage after this is landed is to then enable
check-type-assertions
.If somebody was so inclined they could make the following run for tests as well!
The
errcheck
found a few gotchas in the code base, either we weren't handlingthe error correctly or just blindly ignoring it.
As this is an ongoing experiment I've created a new yaml file to allow us to experiment
with linters whilst not breaking the initial setup.
QA steps