-
Notifications
You must be signed in to change notification settings - Fork 2.8k
lint: reenable revive unused-parameter check #27126
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
|
Looks like there are still some changes needed to environments other than Linux. Once CI is complete I'll address these. |
4e5f4f5 to
460422f
Compare
|
[NON-BLOCKING] Packit jobs failed. @containers/packit-build please check. Everyone else, feel free to ignore. |
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.
Thanks, LGTM. Just one small request: could you please squash all your commits into one?
CI fails are flakes.
4ea8fc4 to
7c96e72
Compare
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.
Thanks, LGTM.
/LGTM
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.
Thank you LGTM, looks like it needs a rebase though
7c96e72 to
adf37da
Compare
adf37da to
89c832e
Compare
|
Should be good for review now. Thanks for taking a look at this |
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
Thanks
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
To get this merged, please retrigger the CI. The test looks like it's flaking—otherwise LGTM.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Honny1, Luap99, medsouz The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@medsouz looks like there is yet another merge conflict so you have to rebase again. I try to to get this merged quickly once you push again so you hopefully don't have to deal with that again, sorry about that. |
Signed-off-by: Matt Souza <medsouz99@gmail.com>
89c832e to
090304a
Compare
|
No worries @Luap99, merging something that touches this many files is always a pain. 😆 Should be good to go now but I'll keep an eye on CI and any future merges to main today to keep it in a ready state. |
|
/lgtm |
d0212f9
into
containers:main
To help maintain the sanity of whoever has to review this PR I've limited my changes to only change argument names violating this linter to
_. There seems to be a few spots that could be cleaned up further but that feels out of scope for a change this large.Resolves #27111
Does this PR introduce a user-facing change?