Skip to content

linter:usrmerge: Improve error message to list files #1863

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

Merged

Conversation

justinvreeland
Copy link
Contributor

@justinvreeland justinvreeland commented Mar 20, 2025

Looks like this:

Package contains paths in violation of usrmerge:
bin
bin/test
sbin
sbin/test
usr/sbin
usr/sbin/test

@justinvreeland justinvreeland force-pushed the jvreeland/linter-message branch from c0e6a9f to c8c4e1a Compare March 20, 2025 16:41
@justinvreeland justinvreeland marked this pull request as ready for review March 20, 2025 16:44
@justinvreeland justinvreeland force-pushed the jvreeland/linter-message branch 2 times, most recently from 2b79445 to c8c4e1a Compare March 20, 2025 17:09
@justinvreeland justinvreeland requested a review from smoser March 20, 2025 17:10
smoser
smoser previously approved these changes Mar 20, 2025
Copy link
Contributor

@smoser smoser left a comment

Choose a reason for hiding this comment

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

I think the code on lines 803-810 is effectively unreachable . unless WalkDir searches depth-first.

But this is better than it was.

I would prefer a list of the things that are bad. But Justin says this is better.

@justinvreeland
Copy link
Contributor Author

Most of our other linters exit on the first error they find and I don't see a benefit to listing the files beyond the first one you encounter since the remedy is the same. I'll check if that's unreachable and probably remove that section if it is.

@justinvreeland
Copy link
Contributor Author

need to remove the schema updates and such that snuck in. idk why these aren't update in the pr that should've changed them.

Looks like this:

```
Package contains paths in violation of usrmerge:
bin
bin/test
sbin
sbin/test
usr/sbin
usr/sbin/test
```
@justinvreeland justinvreeland force-pushed the jvreeland/linter-message branch from a1a15f6 to ccd82b8 Compare May 14, 2025 15:40
@smoser
Copy link
Contributor

smoser commented May 20, 2025

i think this looks fine. the only question, from reading this I think that the commit message is out dated.

you would not see directories listed, right? only non-directories. Ie, in the following no usr/sbin or bin or usr/sbin in the beolow.

Package contains paths in violation of usrmerge:
bin
bin/test
sbin
sbin/test
usr/sbin
usr/sbin/test

@justinvreeland
Copy link
Contributor Author

i think this looks fine. the only question, from reading this I think that the commit message is out dated.

Ahh yeah I need to update that or fix it in the merge command.

in the following no usr/sbin or bin or usr/sbin in the beolow.

Every other line is a directory in that snippet?

@justinvreeland
Copy link
Contributor Author

Oh of course I updated the commit message not the github summary dislike

@justinvreeland justinvreeland changed the title linter:usrmerge: Improve error message linter:usrmerge: Improve error message to list files May 20, 2025
@justinvreeland justinvreeland merged commit efac382 into chainguard-dev:main May 22, 2025
54 checks passed
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.

2 participants