Rebased: Additional groups lookup #603
Conversation
|
@dqminh Thanks a lot for doing this. I tested it and it works fine :) |
dqminh
force-pushed
the
mrunalp-add_groups_lookup
branch
from
16e6e87
to
3253691
Compare
|
@LK4D4 PTAL |
| break | ||
| } | ||
| } | ||
| if !found { |
There was a problem hiding this comment.
Here found can't be true, because you're break from loop when set true. So you don't need this variable at all.
There was a problem hiding this comment.
I think you are right. The logic here can be reworked to make it clearer.
dqminh
force-pushed
the
mrunalp-add_groups_lookup
branch
from
3253691
to
0576622
Compare
| } | ||
| gidMap[gid] = struct{}{} | ||
| } | ||
| } |
There was a problem hiding this comment.
@LK4D4 I reworked the loop so it's clearer now that we either look at the matched groups or save the new group id. PTAL.
|
LGTM |
|
ping @crosbymichael |
|
@crosbymichael @vmarmol PTAL |
| @@ -85,7 +85,7 @@ type Config struct { | |||
|
|
|||
| // AdditionalGroups specifies the gids that should be added to supplementary groups | |||
| // in addition to those that the user belongs to. | |||
| AdditionalGroups []int `json:"additional_groups"` | |||
There was a problem hiding this comment.
Any reason for the change from int to string for groups?
There was a problem hiding this comment.
i think it's because Docker wants to specify group as either gid or name.
There was a problem hiding this comment.
|
This looks good, but needs a rebase. |
|
Going to rebase asap |
Signed-off-by: Mrunal Patel <mrunalp@gmail.com>
Signed-off-by: Mrunal Patel <mrunalp@gmail.com>
Signed-off-by: Mrunal Patel <mrunalp@gmail.com>
dqminh
force-pushed
the
mrunalp-add_groups_lookup
branch
from
0576622
to
1199ab6
Compare
This parses group file only once to process a list of groups instead of parsing once for each group. Also added an unit test for GetAdditionalGroupsPath Signed-off-by: Daniel, Dao Quang Minh <dqminh89@gmail.com>
dqminh
force-pushed
the
mrunalp-add_groups_lookup
branch
from
1199ab6
to
d4ece29
Compare
|
@vmarmol rebased. PTAL |
|
LGTM |
Rebased: Additional groups lookup
|
Why wasn't I ping'd on this? This API isn't right, we need an |
Original PR #559:
This carries #559, did a rebase and implement the group lookup by scanning through the group file only once.
@mrunalp Can you take a look if this looks good ? I cant open the PR back to the original because of the rebase.