Skip to content

Enable errcheck everywhere and fix or silent remaining issues #1987

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
Merged
denik merged 30 commits into from
Dec 11, 2024
Merged

Enable errcheck everywhere and fix or silent remaining issues #1987

denik merged 30 commits into from
Dec 11, 2024

Conversation

@denik
Copy link
Contributor

@denik denik commented Dec 9, 2024 •
edited
Loading

Changes

Enable errcheck linter for the whole codebase.

Fix remaining complaints:

  • If we can propagate error to caller, do that
  • If we writing to stdout, continue ignoring errors (to avoid crashing in "cli | head" case)
  • Add exception for cobra non-critical API such as MarkHidden/MarkDeprecated/RegisterFlagCompletionFunc. This keeps current code and behaviour, to be decided later if we want to change this.
  • Continue ignoring errors where that is desired behaviour (e.g. git.loadConfig).
  • Continue ignoring errors where panicking seems riskier than ignoring the error.
  • Annotate cases in libs/dyn with //nolint:errcheck - to be addressed later.

Note, this PR is not meant to come up with the best strategy for each case, but to be a relative safe change to enable errcheck linter.

Tests

Existing tests.

@denik denik temporarily deployed to test-trigger-is December 9, 2024 12:28 — with GitHub Actions Inactive
@denik denik temporarily deployed to test-trigger-is December 9, 2024 12:28 — with GitHub Actions Inactive
Base automatically changed from errcheck-tests to main December 9, 2024 12:56
@denik denik force-pushed the aider-fix-errcheck-squashed branch from 5b90bc7 to 16ae671 Compare December 9, 2024 12:58
@denik denik temporarily deployed to test-trigger-is December 9, 2024 12:58 — with GitHub Actions Inactive
@denik denik marked this pull request as ready for review December 9, 2024 12:58
@denik denik temporarily deployed to test-trigger-is December 9, 2024 12:59 — with GitHub Actions Inactive
@denik denik temporarily deployed to test-trigger-is December 9, 2024 13:05 — with GitHub Actions Inactive
@denik denik temporarily deployed to test-trigger-is December 9, 2024 13:05 — with GitHub Actions Inactive
@denik denik temporarily deployed to test-trigger-is December 9, 2024 13:12 — with GitHub Actions Inactive
@denik denik temporarily deployed to test-trigger-is December 9, 2024 13:12 — with GitHub Actions Inactive
@denik denik changed the title Enable errcheck everywhere and fix remaining issues Enable errcheck everywhere and fix or silent remaining issues Dec 9, 2024
@denik denik temporarily deployed to test-trigger-is December 9, 2024 13:22 — with GitHub Actions Inactive
@denik denik temporarily deployed to test-trigger-is December 9, 2024 13:22 — with GitHub Actions Inactive
@denik denik temporarily deployed to test-trigger-is December 9, 2024 13:24 — with GitHub Actions Inactive
@denik denik temporarily deployed to test-trigger-is December 9, 2024 13:24 — with GitHub Actions Inactive
@denik denik temporarily deployed to test-trigger-is December 9, 2024 13:30 — with GitHub Actions Inactive
@denik denik temporarily deployed to test-trigger-is December 9, 2024 13:30 — with GitHub Actions Inactive
@denik denik temporarily deployed to test-trigger-is December 9, 2024 13:33 — with GitHub Actions Inactive
@denik denik temporarily deployed to test-trigger-is December 9, 2024 13:33 — with GitHub Actions Inactive
@denik denik temporarily deployed to test-trigger-is December 9, 2024 13:41 — with GitHub Actions Inactive
@denik denik temporarily deployed to test-trigger-is December 9, 2024 13:41 — with GitHub Actions Inactive
@denik denik temporarily deployed to test-trigger-is December 9, 2024 13:45 — with GitHub Actions Inactive
@denik denik temporarily deployed to test-trigger-is December 9, 2024 13:46 — with GitHub Actions Inactive
pietern
pietern reviewed Dec 9, 2024
View reviewed changes
andrewnester
andrewnester reviewed Dec 9, 2024
View reviewed changes
Copy link
Contributor

@andrewnester andrewnester left a comment

Choose a reason for hiding this comment

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

@pietern can you take a look specifically at dyn library changes?

@denik denik temporarily deployed to test-trigger-is December 10, 2024 10:17 — with GitHub Actions Inactive
denik added 21 commits December 11, 2024 09:55
@denik
ignore error from MarkDeprecated
b34a1e8
@denik
ignore errors from RegisterCompletionFunc
5f12b47
@denik
do not panic on write errosr
1fbe51d
@denik
clean up
3af2a09
@denik
ignore err where it was intended
8e77be3
@denik
rm risky panics
207c5fd
@denik
rm risky panics
65b4f78
@denik
clean up
3063835
@denik
clean ups
0c5fa24
@denik
fix assignment mismatch
a05f0c0
@denik
fixes
e1c607b
@denik
keep ignoring errors that were ignored before
cc7a46d
@denik
refactor: Remove error handling for Set method calls
3b99cdf
@denik
Keep libs/dyn unchanged; annotate with //nolint:errcheck
16d412a
@denik
annotate with nolint:errcheck
d59dd39
@denik
fix: Correct flag marking for verbose flag in deploy command
e4352e8
@denik
refactor: Remove unnecessary error assignments from MarkHidden calls
010ce1e
@denik
refactor: Simplify flag deprecation and completion registration
0832ccd
@denik
refactor: Remove unnecessary error discards from RegisterFlagCompleti…
ee67bb3 
…onFunc calls
@denik
exclude cobra functions from errcheck
09a2a40
@denik
Add MarkFlagRequired to exceptions
883a1c3
@denik denik force-pushed the aider-fix-errcheck-squashed branch from 8eadf5c to 883a1c3 Compare December 11, 2024 08:56
@denik denik temporarily deployed to test-trigger-is December 11, 2024 08:56 — with GitHub Actions Inactive
@denik denik temporarily deployed to test-trigger-is December 11, 2024 08:56 — with GitHub Actions Inactive
@github-actions
Copy link

github-actions bot commented Dec 11, 2024

If integration tests don't run automatically, an authorized user can run them manually by following the instructions below:

Trigger:
go/deco-tests-run/cli

Inputs:

  • PR number: 1987
  • Commit SHA: 883a1c3b11e3155388400547f71a2d1b7d4cd3f0

Checks will be approved automatically on success.

@eng-dev-ecosystem-bot
Copy link
Collaborator

eng-dev-ecosystem-bot commented Dec 11, 2024

Test Details: go/deco-tests/12272889993

@denik denik merged commit 8d5351c into main Dec 11, 2024
10 of 11 checks passed
@denik denik deleted the aider-fix-errcheck-squashed branch December 11, 2024 12:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pietern pietern pietern approved these changes

@andrewnester andrewnester andrewnester approved these changes

@shreyas-goenka shreyas-goenka Awaiting requested review from shreyas-goenka shreyas-goenka is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@denik @eng-dev-ecosystem-bot @pietern @andrewnester