feat: add golangci-lint #48
Conversation
main.go
Outdated
| cli.ContainerRemove(context.Background(), container.ID, types.ContainerRemoveOptions{RemoveVolumes: true, Force: true}) | ||
| err := cli.ContainerRemove(context.Background(), container.ID, types.ContainerRemoveOptions{RemoveVolumes: true, Force: true}) | ||
| if err != nil { | ||
| log.Println(err) |
There was a problem hiding this comment.
Do we want to continue here to avoid incrementing the number of deleted containers?
There was a problem hiding this comment.
If I am nitpicking, I would probably add this in a follow-up bugfix PR, to have a slightly better Git history and keep this PR here focused on the linting change 🙂
There was a problem hiding this comment.
Mmmm I think it is actually part of the change: handle the error, which was not handled in the past
There was a problem hiding this comment.
Which number of deleted containers we would have before? Probably no output, since it would fail? Or we would have the wrong number?
There was a problem hiding this comment.
The error was swallowed, the execution continued adding a container to the map.
Being strict, it could be considered a fix, yeah
There was a problem hiding this comment.
But we can also be pragmatic and fix it here. I think having the discussion was more important than the detail where we fix it (as long as we fix it).
There was a problem hiding this comment.
I'm unsure what the expected functionality should be at this time, happy to discuss it further in another PR as there are a handful of unhandled errors.
There was a problem hiding this comment.
LGTM, although we left a a few comments that need to be addressed before the final approval.
Could you please also resolve the conflicts after recent merges?
Thanks for your time @hhsnopek!
hhsnopek
force-pushed
the
golangci-lint
branch
from
adaa617
to
f910215
Compare
|
@mdelapenya @kiview I apologize for the force push, but I've resolved the comments which means I've removed the additional error handling/logs in-favor of keeping the scope to adding golangci-lint. |
hhsnopek
requested review from
mdelapenya and
kiview
and removed request for
mdelapenya and
kiview
* main: feat: add golangci-lint (testcontainers#48) chore: add unit tests for Prune function (testcontainers#46) chore(ci): only push Docker image on tags (testcontainers#47) chore: bump build system versions (testcontainers#45) chore: rename branch to main (testcontainers#44)
Context
This pull request introduces
golangci-lintinto the existing CI/CD Pipeline to increase the project's code health. Applyinggo fmtto all files and fixes a lot of missing error handling.