-
Notifications
You must be signed in to change notification settings - Fork 38
Report _all_ lint failures, and fix the outstanding ones #429
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
Conversation
|
✅ A new PR has been created in buildah to vendor these changes: containers/buildah#6465 |
Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>
Otherwise, a CI run reports 3 issues, we fix them, and the CI run reports _next_ 3 issues. That's a waste of time all around, and it does not allow judging the scale of the issue. Signed-off-by: Miloslav Trmač <[email protected]>
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.
LGTM
| path: libimage | ||
| text: LookupImage | ||
|
|
||
| issues: |
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.
+1 I did this in podman a long time ago, no idea why they choose such default really
containers/podman@93c35a7
It always just leads to the wrong assumption of let me just fix these three issues really quick.
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.
LGTM
even though I'm a day late and a dollar short for this review.
Thanks @mtrmac !
Currently,
golangci-lintdoes not report all failures if there are too many, or too many duplicates.So, e.g. in #425 , we see some failures, fix them ( #427 ) — and then there are more failures to fix.
Avoid that waste by reporting all failures immediately; and fix the outstanding ones.
@TomSweeneyRedHat PTAL