lxd/idmap/linux: address golanglint-ci issues
#16181
Conversation
simondeziel
force-pushed
the
idmapset-linux
branch
4 times, most recently
from
d910bd1 to
7ac9bb1
Compare
simondeziel
force-pushed
the
idmapset-linux
branch
from
7ac9bb1 to
238ff89
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR addresses various golangci-lint issues throughout the LXD idmap codebase by fixing error messages, adding documentation, improving naming conventions, refactoring code for better maintainability, and adding comprehensive test coverage.
- Fixes error message capitalization inconsistencies and adds comprehensive test coverage
- Adds proper documentation to exported functions and improves code organization
- Refactors duplicate code patterns and improves function naming conventions
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| lxd/idmap/parse_linux_test.go | Adds comprehensive test suite for ParseRawIdmap function |
| lxd/idmap/parse_linux.go | Fixes error message capitalization consistency |
| lxd/idmap/idmapset_linux_test.go | Adds extensive test coverage for various idmap functions |
| lxd/idmap/idmapset_linux.go | Major refactoring with documentation, naming improvements, and code consolidation |
| fuidshift/main_shift.go | Updates function calls to match renamed methods |
| .golangci.yaml | Removes exclusion for idmapset_linux.go from linting |
simondeziel
force-pushed
the
idmapset-linux
branch
from
238ff89 to
c2a27b2
Compare
|
|
||
| scanner := bufio.NewScanner(f) | ||
| for scanner.Scan() { | ||
| // Skip comments |
There was a problem hiding this comment.
Did you check history of this change and why it was added?
There was a problem hiding this comment.
I had not done it because the files in questions are coming from the procfs which contains stuff build dynamically by the kernel. Having a comment show up there would be weird.
That said, I tracked it down to being introduced by commit dc64c84 in 2017 and appears to have been a copy-n-paste from the getFromShadow() function that predates it. The shadow variant deals with files created by humans so comments are possible.
Lastly, I did not include it in the final list of tests but even if comments are somehow present in what the getFromProc reads, it has logic to skip bogus lines. I've now reinstate an unlikely test where a comment is present.
There was a problem hiding this comment.
and appears to have been a copy-n-paste from the
getFromShadow()function that predates it.
Ah yeah seems likely indeed cheers
|
@simondeziel are you able to reopen this for the cla check? |
Signed-off-by: Simon Deziel <[email protected]>
Signed-off-by: Simon Deziel <[email protected]>
Signed-off-by: Simon Deziel <[email protected]>
Signed-off-by: Simon Deziel <[email protected]>
Signed-off-by: Simon Deziel <[email protected]>
Signed-off-by: Simon Deziel <[email protected]>
Signed-off-by: Simon Deziel <[email protected]>
Signed-off-by: Simon Deziel <[email protected]>
This reverts commit 8002fc0. Signed-off-by: Simon Deziel <[email protected]>
Signed-off-by: Simon Deziel <[email protected]>
Signed-off-by: Simon Deziel <[email protected]>
Signed-off-by: Simon Deziel <[email protected]>
Signed-off-by: Simon Deziel <[email protected]>
Signed-off-by: Simon Deziel <[email protected]>
Signed-off-by: Simon Deziel <[email protected]>
…inimal size Signed-off-by: Simon Deziel <[email protected]>
Signed-off-by: Simon Deziel <[email protected]>
Signed-off-by: Simon Deziel <[email protected]>
Signed-off-by: Simon Deziel <[email protected]>
Signed-off-by: Simon Deziel <[email protected]>
Signed-off-by: Simon Deziel <[email protected]>
simondeziel
force-pushed
the
idmapset-linux
branch
from
c2a27b2 to
611e146
Compare
|
With that PR, I wanted to squash the 6 integer overflow alerts that CodeQL found in that file. Despite not indicating the PR was achieving this, it seems like it did once merged into We have quite a few other alerts to tend to but slightly less than before :) |
2 participants
No description provided.