Bug 1421856

Summary:	Review Request: etcd-container - system container for etcd
Product:	[Fedora] Fedora	Reporter:	Yu Qi Zhang <jzehrarnyg>
Component:	Package Review	Assignee:	Tomas Tomecek <ttomecek>
Status:	CLOSED NEXTRELEASE	QA Contact:	Fedora Extras Quality Assurance <extras-qa>
Severity:	medium	Docs Contact:
Priority:	unspecified
Version:	25	CC:	container-review, package-review, ttomecek
Target Milestone:	---	Flags:	ttomecek: fedora-review+
Target Release:	---
Hardware:	All
OS:	Linux
Whiteboard:
Fixed In Version:		Doc Type:	If docs needed, set a value
Doc Text:		Story Points:	---
Clone Of:		Environment:
Last Closed:	2017-02-24 17:07:46 UTC	Type:	---
Regression:	---	Mount Type:	---
Documentation:	---	CRM:
Verified Versions:		Category:	---
oVirt Team:	---	RHEL 7.3 requirements from Atomic Host:
Cloudforms Team:	---	Target Upstream Version:
Embargoed:
Bug Depends On:
Bug Blocks:	177841

Description Yu Qi Zhang 2017-02-13 21:13:11 UTC

Dockerfile URL: https://github.com/projectatomic/atomic-system-containers/blob/master/etcd/Dockerfile

Source: https://github.com/projectatomic/atomic-system-containers/tree/master/etcd

Description: 
etcd is a key-value store for shared configuration and service discovery. This package is intended to be ran as a system container.

More information on system containers here: http://www.projectatomic.io/blog/2016/09/intro-to-system-containers/
A quick guide: https://github.com/yuqi-zhang/atomic-system-containers-quickstart

Fedora Account System Username: yzhang

Comment 1 Tomas Tomecek 2017-02-15 13:10:50 UTC

Same as flannel, please pin base image to latest stable tag:

> FROM fedora

https://fedoraproject.org/wiki/Container:Guidelines#CMD_.2F_ENTRYPOINT


https://github.com/projectatomic/atomic-system-containers/blob/master/etcd/Dockerfile#L13
> RUN dnf -y install etcd hostname && \

This also holds for flannel: please install packages without documentation as described in "Container best practices":

http://docs.projectatomic.io/container-best-practices/#_clear_packaging_caches_and_temporary_package_downloads


It would be also helpful to indicate which ports are meant to be exposed (you write that the container is meant to run as system container, but one can still run it within docker, right?)


> # git clone https://github.com/aveshagarwal/etcd-container

`git clone` in readme seems to be out of date.


Same for documentation, it would be really helpful for users to have something. But I guess since there are no formal guidelines for this, let's postpone (same for flannel), there is readme in the repo, should be enough for now.

Comment 2 Yu Qi Zhang 2017-02-16 19:28:22 UTC

Thanks for the review! I've made the fixes to etcd and flannel here: https://github.com/projectatomic/atomic-system-containers/pull/32

Comment 3 Tomas Tomecek 2017-02-17 13:30:19 UTC

I don't want to nitpick, but it's `maintainer`, with lower-case 'm'. Also, please  use EXPOSE instruction inside etcd Dockerfile to indicate, which ports are meant to be exposed.

These are really minor, will grant review+ once done.


Since you are not sponsored yet, have you tried to get a sponsor?


Since are not set as maintainer inside Dockerfile, is Giuseppe okay with this? Will both of you maintain the image in Fedora?

Comment 4 Yu Qi Zhang 2017-02-21 16:46:25 UTC

Fixed in: https://github.com/projectatomic/atomic-system-containers/commit/759decde35f539e743689020e251f9a8fcc5eb07

I have talked to Adam Miller about sponsoring. As for maintainer, technically projectatomic as an organization will be the maintainer. I've talked to Giuseppe and he's ok with being the listed maintainer for now.

Comment 5 container-review 2017-02-21 23:27:16 UTC

Sponsored.

Comment 6 Tomas Tomecek 2017-02-22 08:59:05 UTC

well done!

Comment 7 Gwyn Ciesla 2017-02-22 16:55:42 UTC

Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/docker/etcd