overlay: refactor getStagingDir helper #2107
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: giuseppe The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
0bd5b74 to
ceab862
Compare
|
@rhatdan thanks for the review. Addressed the comments |
drivers/overlay/overlay.go
Outdated
| func (d *Driver) dir2(id string, useImageStore bool) (string, string, bool) { | ||
| var homedir string | ||
|
|
||
| func (d *Driver) homeDir(useImageStore bool) string { |
There was a problem hiding this comment.
I’m confused by the terminology. We have d.homeDir and d.home, and those are different values with different semantics.
What do the values actually mean?
(I mean, I’m generally confused and scared by these code paths and options and indirections. This is just a very clear manifestation of my general confusion. But, still.)
pruneStagingDirectories hard-codes filepath.Join(d.home, stagingDir), not using the useImageStore condition. Which of the two is correct?
There was a problem hiding this comment.
thanks, I've renamed the function, hopefully it is clearer now.
pruneStagingDirectories was using the wrong location, I've fixed this too
There was a problem hiding this comment.
I am still generally confused ;), but making the names are much more distinct does help.
simplify the implementation of getStagingDir() to just create a new staging directory and drop the check whether the correct staging base directory as anyway the rename operation would fail later. Closes: containers#2092 Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
ceab862 to
7f30233
Compare
|
LGTM |
simplify the implementation of getStagingDir() to just create a new staging directory and drop the check whether the correct staging base directory as anyway the rename operation would fail later.
Closes: #2092