Skip to content
This repository was archived by the owner on Oct 10, 2020. It is now read-only.

Minor fix to install logic#954

Closed
yuqi-zhang wants to merge 1 commit intoprojectatomic:masterfrom
yuqi-zhang:system-install
Closed

Minor fix to install logic#954
yuqi-zhang wants to merge 1 commit intoprojectatomic:masterfrom
yuqi-zhang:system-install

Conversation

@yuqi-zhang
Copy link
Contributor

Add check for image existing in local backends, which allows for
'atomic.type=system' labelled containers to be installed without
an extra remote check.

Signed-off-by: Yu Qi Zhang jerzhang@redhat.com

Add check for image existing in local backends, which allows for
'atomic.type=system' labelled containers to be installed without
an extra remote check.

Signed-off-by: Yu Qi Zhang <jerzhang@redhat.com>
@rhatdan
Copy link
Member

rhatdan commented Mar 27, 2017

@yuqi-zhang Isn't this a problem though, that you have already pulled the image and stored it probably in docker, we want it directly installed in --ostree.

@yuqi-zhang
Copy link
Contributor Author

Sorry, I'm not quite sure what you mean, do you mean a case where the image is locally in docker already, but has a 'atomic.type=system' label? Then yes that is an edge case where if there is a local docker image AND the user doesn't specify --system or --storage, the CLI would assume the image should be installed as a docker container. This behavior did not change from the old code.

I feel like we should allow users to do that, since for an image with 'atomic.type=system' to have been pulled to docker, the user must have specified --storage=docker to override it. The user may want to run it as a docker container for other reasons.

Locally built images are another edge case, and perhaps should be re-considered.

@rhatdan
Copy link
Member

rhatdan commented Mar 28, 2017

I have no problem with the local case. But I am more interested in the remote case.

A user doing atomic install registry.access.redhat.com/etcd-system

Should be installed as a system container.
Skopeo has the means to look at the json file on registry.access.redhat.com and see that etcd-system has the proper labels to be installed as a system container.

@yuqi-zhang
Copy link
Contributor Author

I have tested that case, it would work if registry.access.redhat.com/etcd-system has 'atomic.type=system'. I did not change this logic.

The only caveat is that if docker isn't running, since the remote inspect calls all the way down to https://github.com/projectatomic/atomic/blob/master/Atomic/util.py#L63 it will fail with NoDockerDaemon: The docker daemon does not appear to be running. But this is also the case in the previous code. I will talk to @baude about it.

@rhatdan
Copy link
Member

rhatdan commented Mar 28, 2017

Excellent, if that use case works.

@rhatdan
Copy link
Member

rhatdan commented Apr 13, 2017

@giuseppe PTAL

@giuseppe
Copy link
Collaborator

this change LGTM, we shouldn't require Docker running to get install a system container

@rhatdan
Copy link
Member

rhatdan commented Apr 15, 2017

@rh-atomic-bot r+ 6563df8

@rh-atomic-bot
Copy link

⌛ Testing commit 6563df8 with merge c134ee5...

@rh-atomic-bot
Copy link

☀️ Test successful - status-redhatci
Approved by: rhatdan
Pushing c134ee5 to master...

Copy link

@TomSweeneyRedHat TomSweeneyRedHat left a comment

Choose a reason for hiding this comment

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

LGTM

@yuqi-zhang yuqi-zhang deleted the system-install branch May 4, 2017 20:00
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants