-
Notifications
You must be signed in to change notification settings - Fork 18.6k
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
Fix missing Init Binary in docker info output #32469
Fix missing Init Binary in docker info output #32469
Conversation
{ | ||
config: &Config{}, | ||
expectedInitPath: "docker-init", | ||
}, |
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.
Perhaps we should have a test-case where InitPath
is set as well?
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.
@thaJeztah yep, missed that out 😬
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.
@thaJeztah Added that too.
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.
oh, I'm gonna be a PITA; can you add one with both InitPath
and DefaultInitBinary
set, so that we test that InitPath
correctly overrides DefaultInitBinary
?
And when you added that, can you "squash" your commits, so that there's only a single commit in this PR? 😅
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.
@thaJeztah lol! that's alright 😁 Added another test-case and squashed the commits.
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.
Perfect, thank you so much!
75fd141
to
5d7d2d2
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.
LGTM 🦁
/cc @mlaventure
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.
left one more question, but LGTM once that's done!
Thanks so much for picking this up!
{ | ||
config: &Config{}, | ||
expectedInitPath: "docker-init", | ||
}, |
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.
oh, I'm gonna be a PITA; can you add one with both InitPath
and DefaultInitBinary
set, so that we test that InitPath
correctly overrides DefaultInitBinary
?
And when you added that, can you "squash" your commits, so that there's only a single commit in this PR? 😅
- Moved DefaultInitBinary from daemon/daemon.go to daemon/config/config.go since it's a daemon config and is referred in config package files. - Added condition in GetInitPath to check for any explicitly configured DefaultInitBinary. If not, the default value of DefaultInitBinary is returned. - Changed all references of DefaultInitBinary to refer to the variable from new location. - Added TestCommonUnixGetInitPath to test for the various values of GetInitPath. Fixes moby#32314 Signed-off-by: Sunny Gogoi <indiasuny000@gmail.com>
5d7d2d2
to
17b1288
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.
LGTM (if green 😅)
Thank you!
daemon/config/config.go since it's a daemon config and is referred in
config package files.
DefaultInitBinary. If not, the default value of DefaultInitBinary is
returned.
from new location.
DefaultInitBinary.
Fixes #32314
Signed-off-by: Sunny Gogoi indiasuny000@gmail.com
- What I did
Added the missing Init Binary info in
docker info
output.- How I did it
Details are in commit body.
- How to verify it
Verify by running
docker info
and checking the value ofInit Binary
in the output.- Description for the changelog
Fix missing Init Binary in docker info output.
- A picture of a cute animal (not mandatory but encouraged)