Bug 1421856 - Review Request: etcd-container - system container for etcd
Summary: Review Request: etcd-container - system container for etcd
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: 25
Hardware: All
OS: Linux
unspecified
medium
Target Milestone: ---
Assignee: Tomas Tomecek
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: FE-NEEDSPONSOR
TreeView+ depends on / blocked
 
Reported: 2017-02-13 21:13 UTC by Yu Qi Zhang
Modified: 2017-02-24 17:07 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2017-02-24 17:07:46 UTC
Type: ---
Embargoed:
ttomecek: fedora-review+


Attachments (Terms of Use)

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


Note You need to log in before you can comment on or make changes to this bug.