-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
WithAppendAdditionalGroups: better /etc/group handling #9494
Conversation
oci/spec_opts.go
Outdated
if err != nil { | ||
return err | ||
if !os.IsNotExist(err) { | ||
return err | ||
} |
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 make it a real one-line change;
if err != nil && !os.IsNotExist(err) {
(could also use errors.Is(err, fs.ErrNotExist)
/ errors.Is(err, os.ErrNotExist)
)
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.
In other parts of the code os.IsNotExist(err)
is used, I can switch though if you really want
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.
I'm fine with either os.IsNotExist(err)
or errors.Is()
; ISTR os.IsNotExist
also had some code to handle some corner-cases for cgroups (which isn't handled by errors.Is
), so changing it in other places would depend on what it's used for.
Was mostly looking at the "make it a one-line", as it slightly reduces cognitive overload (in already complex code). Not an absolute dealbreaker, but as we're changing it 🤷♂️
5745d20
to
ee4b602
Compare
ee4b602
to
b876a37
Compare
In a follow-up we should probably also look at other parts of the code that read |
b876a37
to
1cf3094
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, but left one more comment 🙈 (sorry, didn't notice that earlier)
oci/spec_opts.go
Outdated
var ugroups []user.Group | ||
var groupErr error | ||
ugroups, groupErr = user.ParseGroupFile(gpath) | ||
if groupErr != nil && !os.IsNotExist(groupErr) { |
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.
Not a show-stopper, but just realised we don't really need the var
declarations here, and could keep the original;
var ugroups []user.Group | |
var groupErr error | |
ugroups, groupErr = user.ParseGroupFile(gpath) | |
if groupErr != nil && !os.IsNotExist(groupErr) { | |
ugroups, groupErr := user.ParseGroupFile(gpath) | |
if groupErr != nil && !os.IsNotExist(groupErr) { |
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.
Done
1cf3094
to
5a3ab59
Compare
Scratch images don't necessarily have the /etc/group file, so we shouldn't fail if opening/parsing it is not needed: if all the group to add are numeric. Signed-off-by: Djordje Lukic <djordje.lukic@docker.com>
5a3ab59
to
1398186
Compare
return err | ||
ugroups, groupErr := user.ParseGroupFile(gpath) | ||
if groupErr != nil && !os.IsNotExist(groupErr) { | ||
return groupErr |
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.
Thinking a bit more about this, I think we want to ignore this error completely, no matter what the error is unless the file is needed. WDYT?
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.
imho it would be good to keep this error check and return early in case of parsing error. looking into user.ParseGroupFile
impl it does not seem to guarantee nil result on error. so the remaining logic would potentially be processing partially parsed results.
Scratch images don't necessarily have the /etc/group file, so we shouldn't fail if opening/parsing it is not needed: if all the group to add are numeric.