Skip to content

Conversation

@rhatdan
Copy link
Member

@rhatdan rhatdan commented Mar 13, 2019

Currently in rootless containers, we end up not using the blob cache.
We also don't store the blob cache based on the users specified graph
storage. This change will cause the cache directory to be stored with
the rest of the containe images.

While doing this patch, I found that we had duplicated GetSystemContext in
two places in libpod. I cleaned this up.

Signed-off-by: Daniel J Walsh dwalsh@redhat.com

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rhatdan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M labels Mar 13, 2019
@@ -127,7 +126,7 @@ func pullCmd(c *cliconfig.PullValues) error {
} else {
authfile := getAuthFile(c.String("authfile"))
spec := image
systemContext := common.GetSystemContext("", authfile, false)
systemContext := image2.GetSystemContext("", authfile, false)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not due to this review, but I think we can drop image2 now, I think the other include ending in image has gone away. (Or maybe I've been staring at the screen too long today and missed it).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image2 is used because they have a variable named image in use.

@mheon
Copy link
Member

mheon commented Mar 13, 2019

LGTM once tests go green

@giuseppe
Copy link
Member

there are some merge commits in the PR from origin/master. Could it be rebased on top of master instead?

@rhatdan
Copy link
Member Author

rhatdan commented Mar 14, 2019

@giuseppe Should be fixed now.

@TomSweeneyRedHat
Copy link
Member

LGTM assuming happy tests and a @mheon head nod.

@mheon
Copy link
Member

mheon commented Mar 14, 2019

Whole bunch of wierd flakes here

@mheon
Copy link
Member

mheon commented Mar 14, 2019

I think the F28 rootless issues might not be flakes. The Ubuntu flake is #2645

@rh-atomic-bot
Copy link
Collaborator

☔ The latest upstream changes (presumably #2696) made this pull request unmergeable. Please resolve the merge conflicts.

@rh-atomic-bot
Copy link
Collaborator

☔ The latest upstream changes (presumably #2789) made this pull request unmergeable. Please resolve the merge conflicts.

@mheon
Copy link
Member

mheon commented Mar 28, 2019

@rhatdan Rebase please
I think I'd like this in 1.2

rhatdan added 2 commits March 29, 2019 08:27
Currently in rootless containers, we end up not using the blob cache.
We also don't store the blob cache based on the users specified graph
storage.  This change will cause the cache directory to be stored with
the rest of the containe images.

While doing this patch, I found that we had duplicated GetSystemContext in
two places in libpod. I cleaned this up.

Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>
Remove references to image2 in source code.  Makes the code
slightly more readable.

Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>
@mheon
Copy link
Member

mheon commented Mar 29, 2019

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 29, 2019
@openshift-merge-robot openshift-merge-robot merged commit 8b5f101 into containers:master Mar 29, 2019
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 26, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants