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

Another bunch of warning cleanups #1364

Merged
merged 18 commits into from
Oct 17, 2022
Merged

Another bunch of warning cleanups #1364

merged 18 commits into from
Oct 17, 2022

Conversation

mtrmac
Copy link
Collaborator

@mtrmac mtrmac commented Sep 26, 2022

… slowly grinding away on golangci-lint outputs with various settings.

Mostly just cleanups, but includes a few real bug fixes — and a change to the golangci-lint configuration.

Note the TarWithOptions and ImagesByTopLayer changes, at least.

Some recurring themes:

  • More error checking, primarily in tests. (I have deferred the harder work of error checking in production)
  • Silence warnings about unused code on macOS.

Eventually we should update to a recent Go, run gofmt, and re-enable all default linters of golangci-lint, some of which are currently disabled.

@mtrmac mtrmac changed the title Another branch of of warning cleanups Another bunch of of warning cleanups Sep 26, 2022
@mtrmac mtrmac changed the title Another bunch of of warning cleanups Another bunch of warning cleanups Sep 26, 2022
@mtrmac mtrmac force-pushed the warnings branch 3 times, most recently from ae35e22 to 7e89278 Compare September 26, 2022 22:03
@mtrmac
Copy link
Collaborator Author

mtrmac commented Sep 26, 2022

(I’d prefer for this to be merged only after the Podman 4.3 branch point, to minimize risk.)

@rhatdan rhatdan changed the title Another bunch of warning cleanups [WIP] Another bunch of warning cleanups Sep 27, 2022
@mtrmac mtrmac force-pushed the warnings branch 5 times, most recently from b4e1078 to 6475496 Compare October 1, 2022 01:07
mtrmac added a commit to mtrmac/buildah that referenced this pull request Oct 1, 2022
... to test containers/storage#1364 + containers/storage#1377 .

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
mtrmac added a commit to mtrmac/libpod that referenced this pull request Oct 1, 2022
... to test containers/storage#1364 + containers/storage#1377 .

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
mtrmac added a commit to mtrmac/buildah that referenced this pull request Oct 1, 2022
... to test containers/storage#1364 + containers/storage#1377 .

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
@mtrmac mtrmac changed the title [WIP] Another bunch of warning cleanups Another bunch of warning cleanups Oct 7, 2022
@mtrmac mtrmac force-pushed the warnings branch 4 times, most recently from 56f8eaa to c60574f Compare October 13, 2022 21:57
Should not change behavior.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Should not change behavior.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Should not change behavior.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Only moves unchanged code, should not change behavior.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Should not change behavior.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Should not change behavior.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
It is only used internally, and this avoids a warning
about a conflict with io.ByteReader.

Should not change behavior.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
> ERRO [runner] Panic: paralleltest: package "main" (isInitialPkg: true, needAnalyzeSource: true): runtime error: index out of range [0] with length 0: goroutine 5859 [running]:
> ...
> github.com/kunwardeep/paralleltest/pkg/paralleltest.isTestFunction(0x1b7d8c0?)
> 	github.com/kunwardeep/paralleltest@v1.0.6/pkg/paralleltest/paralleltest.go:252 +0x165

Should not change behavior.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
It can currently only fail with a broken pipe, or an invalid
pattern; in either case there's no point continuing with the loop.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Have the action handlers return an error value, and let
main() format that error, if any; this avoids duplicated
error formating code in the action handlers, dropping
89 lines.

This might change the error format in some cases (typically
%v vs. %+v).

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Introduce an outputJSON helper to decrease repetition.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
... and remove one WriteFile that was always failing.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
... which allows us to remove an unnecessary NopCloser.

Should not change behavior.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Should not change behavior.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
go { cmd.Run() } means the cmd will be destructed,
and stdin/stdout/stderr closed, concurrently with
other goroutines trying to access stdin/stdout/stderr.

Instead, do it the more traditional way and let the callers
who create those subprocesses explicitly manage their lifetime.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

LGTM

@rhatdan
Copy link
Member

rhatdan commented Oct 17, 2022

LGTM

@rhatdan rhatdan merged commit aba77e1 into containers:main Oct 17, 2022
@mtrmac mtrmac deleted the warnings branch October 17, 2022 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants