Fixes Issue 186: specify custom directories for rhel secrets #191
Conversation
|
Mmm I think this isnt just a carry, instead, this PR should take care of continuing from #187 (comment) implement what Dan and Colin was saying.. |
| if err != nil { | ||
| return nil, err | ||
| } | ||
| overrideSecrets, err := readAll("/etc/container/rhel/secrets", "") |
There was a problem hiding this comment.
If the directory does not exist, we should not fail.
There was a problem hiding this comment.
I think we wanted just /etc/containers/secrets also, no need to hard code rhel.
/etc/containers/secrets
Note plural, which I think we are using for /var/lib/containers
@cgwalters WDYT
There was a problem hiding this comment.
Correct, /etc/containers/secrets
There was a problem hiding this comment.
This does not fail when the directory does not exist.
e.g if /etc/container/rhel/secrets does not exist, then it is just gonna return a slice with baseSecrets and vice versa.
|
We probably want to merge this patch into the secrets patch once it is approved. |
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return append(baseSecrets, overrideSecrets...), nil |
There was a problem hiding this comment.
Does this take care of merging the two directories?
There was a problem hiding this comment.
I mean, when integrating on the slice later in the code...
There was a problem hiding this comment.
yes it works. If both of the directories exists, then in the container rootfs, you will eventually have the union of files in PATH1 and PATH2. With /etc/container/rhel/secrets overriding /usr/share/rhel/secrets.
shishir-a412ed
force-pushed
the
override_rhel_secrets
branch
from
04c8241 to
5dcac6e
Compare
|
@rhatdan Updated the override path |
| return readAll("/usr/share/rhel/secrets", "") | ||
| baseSecrets, err := readAll("/usr/share/rhel/secrets", "") | ||
| if err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
Could we add a little documentation to this error.
return nil, fmt.Errorf("Failed to read /usr/share/rhel/secrets: %s", err.Error())
Signed-off-by: Shishir Mahajan <shishir.mahajan@redhat.com>
shishir-a412ed
force-pushed
the
override_rhel_secrets
branch
from
5dcac6e to
c0543ae
Compare
|
@rhatdan PTAL |
|
LGTM ping @rhatdan @cgwalters |
|
we need this patch fixed in fedora-1.10.3, rhel7-1.10.3, and docker-1.13 also, I'll take care of those once this is merged |
|
LGTM |
|
Lokesh can you merge this with the secrets patch so we have one less patch. |
|
@rhatdan, @lsm5 please do not. The branch docker-1.12 is a freezed branch since upstream 1.12 is GA, we will lose commit history if we squash these changes with the secret patch. The only branch this is going to be squashed is docker-1.13 because upstream hasn't related it yet. Older branch are in a maintaining mode where patches are only added and we never lose commit history by squashing. I'll take care of squashing these changes in docker-1.13, fedora-1.10.3 and rhel-1.10.3 need just a PR to have this changes applied as well. |
|
SGTM. As long as the list of patches we are carrying at projectatomic.io does not grow for the master branch, I am happy |
|
Exactly, the patches in the development branch (in this case docker-1.13) never grow in numbers, that's where I squash commits because we can afford to lose git history from commit to commit. |
Fixes #186
Signed-off-by: Shishir Mahajan shishir.mahajan@redhat.com