Bug 1628914

Unfortunately, the image won't build:
```
Sending build context to Docker daemon 6.144 kB
Step 1/11 : FROM docker://registry.fedoraproject.org/fedora:28
Error parsing reference: "docker://registry.fedoraproject.org/fedora:28" is not a valid repository/tag: invalid reference format
```

The guidelines clearly state that you should use this form for FROM: `FROM registry.example.com/imagename:tag`

There are also some labels missing:

 * maintainer
 * run or usage

Please, consult the guidelines: https://fedoraproject.org/wiki/Container:Guidelines


Not blocking this review:

 * doing update of all packages might break the application, since container images are tested as a unit: changing the unit might produce unknown results
 * that mini-shell script is way too cryptic, why not just doing `cat /extra-packages | xargs dnf install -y`

(In reply to Tomas Tomecek from comment #1)

Thanks for the review! I have updated https://rishi.fedorapeople.org/fedora-toolbox.tar.xz

> Unfortunately, the image won't build:
> ```
> Sending build context to Docker daemon 6.144 kB
> Step 1/11 : FROM docker://registry.fedoraproject.org/fedora:28
> Error parsing reference: "docker://registry.fedoraproject.org/fedora:28" is
> not a valid repository/tag: invalid reference format
> ```
> 
> The guidelines clearly state that you should use this form for FROM: `FROM
> registry.example.com/imagename:tag`

You mean that the docker:// prefix should be removed? It didn't strike me as a problem because it builds locally with 'buildah bud --tag fedora-toolbox:28 .', but I can definitely remove it.

How were you building it?

> There are also some labels missing:
> 
>  * maintainer

The guidelines had a typo, which broke the example, and I wasn't sure if we really wanted somebody's email as a point-of-contact since we already have com.redhat.component.

Fixed.

>  * run or usage

I skipped it because the image is primarily meant to be used with the fedora-toolbox command. I didn't want to encourage people to try it in some other ad-hoc way. I can add a sample 'podman ...' command if you prefer it.

(Isn't 'atomic run' meant to be deprecated?)

> Not blocking this review:
> 
>  * doing update of all packages might break the application, since container
> images are tested as a unit: changing the unit might produce unknown results

Umm... what do you mean by application?

The primary purpose of this image is to be used with https://github.com/debarshiray/fedora-toolbox to setup toolbox containers on Fedora (see the README.md). Setting up a toolbox requires the passwd and shadow-utils packages. The former is installed by the Dockerfile, and the latter comes with the base image. So unless they disappear or suffer a catastrophic breakage, things should mostly work.

On the other hand, not updating the packages on the server would mean that the user would have to perform the same update on their own machine when setting up the toolbox, which seems like a waste of time and bandwidth.

>  * that mini-shell script is way too cryptic, why not just doing `cat
> /extra-packages | xargs dnf install -y`

Sadly, xargs comes from findutils and that's not part of the base fedora image. That means adding a dnf command hard-coded to install findutils, which (a) slows the build due to one more dnf process getting setup and run (b) adds to the list of exceptions that's not covered by the extra-packages file.

If you think the while loop is too cryptic, then I am happy to remove the extra-packages indirection and list all the packages in the Dockerfile itself.

(In reply to Debarshi Ray from comment #2)
> You mean that the docker:// prefix should be removed? It didn't strike me as
> a problem because it builds locally with 'buildah bud --tag
> fedora-toolbox:28 .', but I can definitely remove it.

Yes, the `docker://` prefix.

I used docker to build it -- Fedora OSBS doesn't utilize buildah (yet?).

> How were you building it?
> 
> > There are also some labels missing:
> > 
> >  * maintainer
> 
> The guidelines had a typo, which broke the example, and I wasn't sure if we
> really wanted somebody's email as a point-of-contact since we already have
> com.redhat.component.

Oh, I haven't noticed there is a typo in guidelines.

Feel free to open an issue on our tracker if you think that we should remove the requirement for the maintainer label: https://pagure.io/ContainerSIG/container-sig/issues

> I skipped it because the image is primarily meant to be used with the
> fedora-toolbox command. I didn't want to encourage people to try it in some
> other ad-hoc way. I can add a sample 'podman ...' command if you prefer it.

So please state it then in the usage label: 'This container image is meant to be used with fedora-toolbox command.'

> (Isn't 'atomic run' meant to be deprecated?)

I don't know. The containers team seem to be porting atomic features to podman recently, e.g. https://github.com/containers/libpod/pull/1463

> 
> > Not blocking this review:
> > 
> >  * doing update of all packages might break the application, since container
> > images are tested as a unit: changing the unit might produce unknown results
> 
> Umm... what do you mean by application?
> 
> The primary purpose of this image is to be used with
> https://github.com/debarshiray/fedora-toolbox to setup toolbox containers on
> Fedora (see the README.md). Setting up a toolbox requires the passwd and
> shadow-utils packages. The former is installed by the Dockerfile, and the
> latter comes with the base image. So unless they disappear or suffer a
> catastrophic breakage, things should mostly work.
> 
> On the other hand, not updating the packages on the server would mean that
> the user would have to perform the same update on their own machine when
> setting up the toolbox, which seems like a waste of time and bandwidth.

As I said: the container images are meant to be tested and provided as a unit. When you perform an update inside, you change that unit and make it really hard for the vendor to provide support for such image. If you want the newest packages, you should wait for an updated base image (this is the best practices which I can't find any documentation for).

This was really just a heads-up, feel free to leave it as it is: in the end, it's your image, not mine :)

Also, images are all about the waste of time and bandwidth -- they just make a very good packaging and distribution format and runtime which we all love at the cost of bandwidth, time and bundling mess.

> 
> >  * that mini-shell script is way too cryptic, why not just doing `cat
> > /extra-packages | xargs dnf install -y`
> 
> Sadly, xargs comes from findutils and that's not part of the base fedora
> image. That means adding a dnf command hard-coded to install findutils,
> which (a) slows the build due to one more dnf process getting setup and run
> (b) adds to the list of exceptions that's not covered by the extra-packages
> file.
> 
> If you think the while loop is too cryptic, then I am happy to remove the
> extra-packages indirection and list all the packages in the Dockerfile
> itself.

Oh, I didn't realize that, really good point. Feel free to leave it as it is.


Rishi, I might be off the Internet for several weeks. I don't want you to be blocked on me, hence granting review+. Please, just update the `FROM` line and usage label and we should be good.

Good Evening,

I just tried your container and did not have much success with it.  First, the docker:// is still present so I went a head and remove it within the dockerfile just to continue testing it. I tried multiple way to execute the container and did not find any successful command with fedora-toolbox per README.md


Pasted below is some of my attempts to execute:
brianclark@localhost fedora-toolbox]$ ./fedora-toolbox create
bash: ./fedora-toolbox: No such file or directory
[brianclark@localhost fedora-toolbox]$ ./toolbox create
bash: ./toolbox: No such file or directory
[brianclark@localhost fedora-toolbox]$ docker run -i toolbox
./fedora-toolbox create
/bin/sh: line 1: ./fedora-toolbox: No such file or directory
./fedora-toolbox enter
/bin/sh: line 2: ./fedora-toolbox: No such file or directory
./toolbox create
/bin/sh: line 3: ./toolbox: No such file or directory

Did I miss something?

Also why the dnf upgrade instead of update?

Command I used to build the container:
docker build -t toolbox .

(In reply to Tomas Tomecek from comment #3)

I have updated https://rishi.fedorapeople.org/fedora-toolbox.tar.xz as per the following changes:

> (In reply to Debarshi Ray from comment #2)
> > You mean that the docker:// prefix should be removed? It didn't strike me as
> > a problem because it builds locally with 'buildah bud --tag
> > fedora-toolbox:28 .', but I can definitely remove it.
> 
> Yes, the `docker://` prefix.
> 
> I used docker to build it -- Fedora OSBS doesn't utilize buildah (yet?).

Ok, I see. Removed.

> So please state it then in the usage label: 'This container image is meant
> to be used with fedora-toolbox command.'

Added a 'usage' label as suggested.

> As I said: the container images are meant to be tested and provided as a
> unit. When you perform an update inside, you change that unit and make it
> really hard for the vendor to provide support for such image. If you want
> the newest packages, you should wait for an updated base image (this is the
> best practices which I can't find any documentation for).
> 
> This was really just a heads-up, feel free to leave it as it is: in the end,
> it's your image, not mine :)

Ok, removed the 'dnf -y upgrade'.

> Rishi, I might be off the Internet for several weeks. I don't want you to be
> blocked on me, hence granting review+. Please, just update the `FROM` line
> and usage label and we should be good.

Thanks for the review, Tomas!

(In reply to Brian Clark from comment #4)
> Pasted below is some of my attempts to execute:
> brianclark@localhost fedora-toolbox]$ ./fedora-toolbox create
> bash: ./fedora-toolbox: No such file or directory

Sounds like you don't have the fedora-toolbox script around. You can get it from:
https://github.com/debarshiray/fedora-toolbox

> Also why the dnf upgrade instead of update?

'update' is a deprecated alias for 'upgrade'. Also 'rpm-ostree' favours 'upgrade'.

Filed an SCM admin request:
https://pagure.io/releng/fedora-scm-requests/issue/8260

(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/container/fedora-toolbox

Thanks Gwyn!

It's now built for rawhide:
https://koji.fedoraproject.org/koji/buildinfo?buildID=1147597

I have now requested branches for f29 and f28 too:
https://pagure.io/releng/fedora-scm-requests/issue/8302
https://pagure.io/releng/fedora-scm-requests/issue/8303

... and will build those subsequently.