golint fixes for daemon/ package#15310
Conversation
58e5e3c to
b0a7e06
Compare
|
Edit: linter is on now |
312c843 to
14feaad
Compare
|
fix for windows. kind of concerned that the cross compile did not fail. It should have built everything for Windows, right? |
6cdc4e0 to
ef22f25
Compare
|
I'd suggest exporting execConfig. Any type returned by an exported function should also be exported. |
daemon/container.go
Outdated
There was a problem hiding this comment.
While we're at it, should this change to ErrContainerRootFSReadOnly? Or maybe something a little shorter?
There was a problem hiding this comment.
Does that mean you agree on moving it?
For a rename, ErrRootFSReadOnly? I cannot think of a way to make that much shorter, and I do not think "Container" adds anything informative. Errors like this can also be replaced simply with fmt.errorf as well, because they have no internal variables, only a non-specific message.
There was a problem hiding this comment.
I think it's good practice to export the errors that get returned so package users can check for them, so I'd lean towards leaving it exported. Moving it to the file where it's used sounds okay but I don't think it matters much.
ErrRootFSReadOnly sounds good.
There was a problem hiding this comment.
I think I understand why it does not matter where it is defined. I am still getting used to golang.
daemon/daemon.go
Outdated
There was a problem hiding this comment.
whoops, relic of my understanding embedding. Definitely putting that back. Thanks.
37c215a to
2fb8cbc
Compare
There was a problem hiding this comment.
I'm probably missing something here but : Why is this one rename ?
There was a problem hiding this comment.
There is an already existing function in container_windows.go which causes a conflict if I downcase it.
|
Hum, windows build is failing, |
1cb0903 to
2b08b4d
Compare
|
Yea, bad rebase on my part. Fixing. |
72dfe94 to
67a54a1
Compare
|
@MHBauer would you mind to rebase. I promise to review! |
c3081b5 to
20aa550
Compare
- some method names were changed to have a 'Locking' suffix, as the downcased versions already existed, and the existing functions simply had locks around the already downcased version. - deleting unused functions - package comment - magic numbers replaced by golang constants - comments all over Signed-off-by: Morgan Bauer <mbauer@us.ibm.com>
20aa550 to
abd72d4
Compare
|
LGTM |
|
LGTM, brb, rebasing all my PRs that are affected by me merging this! 🎉 |
golint fixes for daemon/ package
One linter advice yet:
daemon/inspect.go:100:56: exported method ContainerExecInspect returns unexported type *daemon.execConfig, which can be annoying to useSuggestions for addressing it?
There are a surprising (to me) amount of things that became non-exported.
There some comments with "perhaps" in them that I was not sure if I should take action on.
There are some renames with *Locking because all the functions did was add locking around an existing unexported function.
Some of the comments need work as to the exact details.
A partial history of individual commits can be found in my forked repo in the "split-commits" branch.
for #14756