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.

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

Merged
merged 7 commits into from
Feb 11, 2021
Merged

Errcheck linter fixes #12614

merged 7 commits into from
Feb 11, 2021

Conversation

SimonRichardson
Copy link
Member

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 juju errcheck
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 handling
the 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

make static-analysis

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.")
Copy link
Member

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.
Copy link
Contributor

@benhoyt benhoyt left a 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.

@tlm
Copy link
Member

tlm commented Feb 11, 2021

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?

@tlm
Copy link
Member

tlm commented Feb 11, 2021

Looks good Simon

Copy link
Member

@manadart manadart left a 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.

@SimonRichardson
Copy link
Member Author

$$merge$$

@pengale pengale merged commit d8a04d8 into juju:2.9 Feb 11, 2021
@pengale
Copy link
Contributor

pengale commented Feb 11, 2021

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.

@wallyworld wallyworld mentioned this pull request Feb 19, 2021
jujubot added a commit that referenced this pull request Feb 22, 2021
#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
@SimonRichardson SimonRichardson deleted the errcheck-linter-fixes branch July 12, 2021 14:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants