Is there a reason why you cannot use make rpms? + yum install?

BTW you expose ports "EXPOSE 389 636". So you can use argument [-P|--publish-all] with docker run. Or better directly recommend to redirect ports with docker run -p 127.0.0.1:$HOSTPORT:$CONTAINERPORT.

And IMHO it would be good to have readme in the same directory. with description in this tcket.

Is there a reason why you cannot use make rpms? + yum install?

That's something I was considering for a future version, yes. However, in this first iteration:

• It's an extra step that adds little value, as a container is opaque.
• we rely on git master changes in lib389 that are not in rpms, and rpm versioning would need to change very frequently to do it "properly".
• it makes sense to do it as a source build for a poc, especially since I want to trigger this from git changes.
• lets start simple, then add this later.

This space is moving really fast, let's get our support nailed first, then we'll shift it to the rpm strategy and be a bit more conservative.

BTW you expose ports "EXPOSE 389 636". So you can use argument [-P|--publish-all] with docker run. Or better directly recommend to redirect ports with docker run -p 127.0.0.1:$HOSTPORT:$CONTAINERPORT.

That's for an admin to decide, and they can add that to their docker deployment command. :)

And IMHO it would be good to have readme in the same directory. with description in this tcket.

Yes it probably would be a good idea to do that. I can add that.

You probably did not get the point. make rpms would create rpms from git master. it will be equivalent to current version. But Dockerfile will be much simpler.

I would also recommend to use yum-builddep 389-ds-base.spec
instead of listing all build dependencies on two places. It will simplify maintaining of Dockerfile. People tend to forget update the same think on two places.

Thank you very much for Readme.

TL;DR: Our spec file is an unproccesed .in file, which needs to be fixed first before we can really fix this problem in the way you want. Until then, please remember this is a PoC, not a final release.

You probably did not get the point. make rpms would create rpms from git master. it will be equivalent to current version. But Dockerfile will be much simpler.

I got the point :) It's not actually simpler IMO, it adds more for little gain.

Our .spec files relies on ./configure being run, so we need all the build deps anyway. So then we need:

./configure
make rpms
yum install ......389*rpm

Plus, then we need to make the lib389 rpms:

make rpms
yum install .....lib389*rpm

looking at the dockerfile, this would only eliminate a few lines:

make -C lib389 build && \                                                                                                
make -C lib389 install && \   
...
make check && \                                                                                                          
make install && \                                                                                                        
setcap CAP_NET_BIND_SERVICE=pe /usr/sbin/ns-slapd 

But it adds a whole host of complexity IMO.

I would also recommend to use yum-builddep 389-ds-base.spec

yum-builddep does not always work on our spec file as it's an unprocessed .in file, and because of python 3 and what a mess it is, half the packages are macros that don't install.

sudo yum-builddep ds/rpm/389-ds-base.spec.in
No such package(s): ds/rpm/389-ds-base.spec.in

cp ds/rpm/389-ds-base.spec.in /tmp/389-ds-base.spec
sudo yum-builddep /tmp/389-ds-base.spec
error: /tmp/389-ds-base.spec:24: bad %if condition
Bad spec: /tmp/389-ds-base.spec
No uninstalled build requires

This means we need to ./configure to make our spec file "valid", but to do that we need our dependencies. We have a loop now :)

NOTE: This isn't an issue with DNF, but I prefer to target EL7.

instead of listing all build dependencies on two places. It will simplify maintaining of Dockerfile. People tend to forget update the same think on two places.

This is a good point, and I agree with it, but I do worry about our ability for build dep to work.

The second issue with this is now we have:

RUN # install the build deps
RUN # Check out source code
RUN # make and install the rpms

So now there are more actions in the second layer, when pure source our layers are:

RUN # check out source
RUN # install all rpms
RUN # make the source code

A goal is to have it so that we can rebuild just the code layer any below when a change is flagged. Another goal is that a consumer may wish to over-ride the git repo location and build a custom branch for testing. See how now we have to do lots of RPM installs over and over, even though we just changed the code? We have completely messed up our dependency layers!

Thank you very much for Readme.

You're welcome.

Can you amend the documentation (README.md) to specify how data volume(s) should be passed to the container?

TL;DR: Our spec file is an unproccesed .in file, which needs to be fixed first >before we can really fix this problem in the way you want. Until then, please >remember this is a PoC, not a final release.

We had the same chicken and egg problem with sssd spec file template. But we decided to use sed to workaround issues with yum-buildep in our CI script. We really tried to avoid "code duplication"

https://pagure.io/SSSD/sssd/blob/master/f/contrib/ci/deps.sh#_54
and a simillar approach on different place
https://pagure.io/SSSD/sssd/blob/master/f/contrib/fedora/make_srpm.sh#_147

error: /tmp/389-ds-base.spec:24: bad %if condition
Bad spec: /tmp/389-ds-base.spec
No uninstalled build requires

It would be solved by sed. :-)

The biggest benefit of relying on spec file is that you have single source of (build) dependencies on one place. eg. fedora has libnfsidmap-devel and rhel6 has nfs-utils-lib-devel. It simplify testing on various distributions
http://sssd-ci.duckdns.org/logs/job/67/47/summary.html
e.g. You can test with systemd on rhel7 and without systemd on rhel6.
(replace systemd with any other optional feature)

And in case of Dockerfile you would just change base image. Nothing else :-)

TL;DR: Our spec file is an unproccesed .in file, which needs to be fixed first >before we can really fix this problem in the way you want. Until then, please >remember this is a PoC, not a final release.

We had the same chicken and egg problem with sssd spec file template. But we decided to use sed to workaround issues with yum-buildep in our CI script. We really tried to avoid "code duplication"
https://pagure.io/SSSD/sssd/blob/master/f/contrib/ci/deps.sh#_54
and a simillar approach on different place
https://pagure.io/SSSD/sssd/blob/master/f/contrib/fedora/make_srpm.sh#_147

I have considered another option to be:

    sudo yum install -y --skip-broken \
        `grep -E "^(Build)?Requires" ds/rpm/389-ds-base.spec.in svrcore/svrcore.spec lib389/python-lib389.spec | grep -v -E '(name|MODULE)' | awk '{ print $$2 }' | grep -v "^/" | grep -v pkgversion | sort | uniq|  tr '\n' ' '`

Trying to use sed is not so nice IMO. Really there are so many limits with this that it's not funny, and no matter what we do, it's a hack in some way shape or form.

error: /tmp/389-ds-base.spec:24: bad %if condition
Bad spec: /tmp/389-ds-base.spec
No uninstalled build requires

It would be solved by sed. :-)
The biggest benefit of relying on spec file is that you have single source of (build) dependencies on one place. eg. fedora has libnfsidmap-devel and rhel6 has nfs-utils-lib-devel. It simplify testing on various distributions
http://sssd-ci.duckdns.org/logs/job/67/47/summary.html
e.g. You can test with systemd on rhel7 and without systemd on rhel6.
(replace systemd with any other optional feature)
And in case of Dockerfile you would just change base image. Nothing else :-)

However, all of these do no solve the layer ordering issue.

In your proposed method it's

Get source code
Install build requires
Build source
Install RPM and RPM requirements

In mine it's

Install all RPM requirements
Get source code
Build source

You method will have issue of "thrashing" on a docker cache build, where you have to install rpms over and over again every source change. My ordering means you can change source without needing to reinstall RPM's over and over.

Sure, on docker hub it should be a no cache build every time (I hope), but if we want to make this better we need to think of a better way.

Perhaps they layer caching issues isn't a big deal? I'm not sure. For now, there was a reason to my madness, to make it so that a local developer, testing their code, has a fast and seamless build turn around cycle without thrashing.

Can you amend the documentation (README.md) to specify how data volume(s) should be passed to the container?

You can not provide volumes to the Proof Of Concept image: it's ephemeral due to a limitation of docker volumes. This is corrected by #49213 .

Seriously, I can't stress enough: This is meant to be a proof of concept, that we are almost there, but there is that last 20% of polish I need to do. But right now I want to get that ball rolling. It's not a perfect image, it's there so that we can start to polish out the other issues. So don't get your hopes up that this will be ready for FreeIPA/anything else for a period of time yet please :)

Update readme to say that volumes are not currently supported.

Although it is not always needed, it may be convenient to prepare the way to switch the version/tag...?
+ENV container docker \
DS_VERSION=master

+cd ds && \
git checkout ${DS_VERSION} && \

Another nice to have would be a way to clone from a forked git tree?

Please ignore this comment if it is out of focus. :)

Although it is not always needed, it may be convenient to prepare the way to switch the version/tag...?
+ENV container docker \
DS_VERSION=master
+cd ds && \
git checkout ${DS_VERSION} && \

I think we can use ARG instead, as that allows docker build --build-arg to over-ride the value.

Another nice to have would be a way to clone from a forked git tree?
Please ignore this comment if it is out of focus. :)

No, this is exactly the point of this image is to let us try source builds in docker. Being able to specify a forked tree would be really useful.

Thanks for your comments Noriko!

@lslebodn What do you think of this version instead? Thinking about it, layer caching won't help us at all, because we have to run no-cache to get new git content anyways so we always pay this cost.

Looks a little bit better. At least there are not the same dependencies on two places. :-)
I think this version is good enough for initial version.

Just the last nitpick. some line in readme are quite long (much more than 80 collumns.) Diff will be complicated if you fix typo there :-)

But that might be fixed before pushing the patch.

LGTM

I'm assuming the LGTM is an ack?

Instead of parsing the spec file "manually", I suggest using something like this:

$ rpmspec -q --buildrequires 389-ds-base
bzip2-devel
cyrus-sasl-devel
gcc-c++
icu
libdb-devel
libevent-devel
libicu-devel
libtalloc-devel
libtevent-devel
lm_sensors-devel
net-snmp-devel
nspr-devel
nss-devel
openldap-devel
openssl-devel
pam-devel
pcre-devel
python3-devel
svrcore-devel
systemd-devel
systemd-units
tcp_wrappers
zlib-devel

As discussed with @rmeggins on IRC, this sadly doesn't work for the same reason on EL7.

Given we have @lslebodn 's "little ack", and no one else has objections, I'll merge this in 24 hours time if that's okay?

commit 01561a10cf22059bd582306ad3f2cac289e2413e
To ssh://git@pagure.io/389-ds-base.git
1b5a578..01561a1 master -> master

389-ds-base is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in 389-ds-base's github repository.

This issue has been cloned to Github and is available here:
- https://github.com/389ds/389-ds-base/issues/2266

If you want to receive further updates on the issue, please navigate to the github issue
and click on subscribe button.

Thank you for understanding. We apologize for all inconvenience.