Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

samba-dc now uses j2 cli command to support DNS_FORWARDER #106

Merged
merged 2 commits into from
Feb 24, 2023

Conversation

psaxton
Copy link
Contributor

@psaxton psaxton commented Feb 10, 2023

This PR addresses issue #44 and enhances PR #96.

PR #96 introduces the DNS_FORWARDER variables but samba-dc doesn't actually use the jinja2 templating despite the .j2 file extension. This PR moves from using sed to j2cli for templating and completes support for DNS_FORWARDER.

@instantlinux
Copy link
Owner

instantlinux commented Feb 13, 2023

Thanks for the PR. I think image size can be reduced by removing py3-pip package after the install. (Still need py3-setuptools). Can you verify that this works, see how much image size it saves, and update the PR?

RUN apk add ...
      bind bind-libs bind-tools libcrypto1.1 libxml2 tzdata py3-setuptools && \
      apk add --no-cache --virtual .fetch-deps py3-pip && \
      pip install j2cli && \
      apk del .fetch-deps && \
...

@psaxton
Copy link
Contributor Author

psaxton commented Feb 13, 2023 via email

@bhechinger
Copy link
Contributor

Looks like putting apk del py3-pip into the RUN gets us almost back down to the original image size:

test1 is with py3-pip, test2 is with it removed.

14:15:44 in docker-tools/images/samba-dc on  prs/44-support-dns-forwarder [!?] [I] ➜ docker images | grep samba
wonko/samba-dc                      test2             cb4363bc7062   9 seconds ago   172MB
wonko/samba-dc                      test1             5a4bc55db083   3 minutes ago   191MB
instantlinux/samba-dc               latest            c1cbd4e05c39   2 months ago    171MB

@bhechinger
Copy link
Contributor

Hmm, that does appear to break it, however:

Attaching to ad-app-1
ad-app-1  | Set timezone
ad-app-1  | Traceback (most recent call last):
ad-app-1  |   File "/usr/bin/j2", line 5, in <module>
ad-app-1  |     from j2cli import main
ad-app-1  |   File "/usr/lib/python3.10/site-packages/j2cli/__init__.py", line 4, in <module>
ad-app-1  |     import pkg_resources
ad-app-1  | ModuleNotFoundError: No module named 'pkg_resources'

@instantlinux
Copy link
Owner

@bhechinger make sure py3-setuptools is installed before the add/remove of py3-pip. That's where the pkg_resources module lives.

@bhechinger
Copy link
Contributor

bhechinger commented Feb 16, 2023

Adding that package in does make it work, but we're no longer saving as much space. :(

wonko/samba-dc                      test3             29ebbb830041   2 minutes ago   182MB

I guess 9MB is better than none. 🤷‍♂️

Here's the RUN block in its entirety now:

RUN apk add --update --no-cache krb5 ldb-tools samba-dc=$SAMBA_VERSION samba-winbind-clients=$SAMBA_VERSION tdb \
      bind bind-libs bind-tools libcrypto1.1 libxml2 tzdata py3-pip && \
    pip install j2cli && \
    apk del py3-pip && \
    apk add py3-setuptools && \
    chmod 0755 /usr/local/bin/entrypoint.sh

@instantlinux
Copy link
Owner

I pushed an update that adds DNS_FORWARDER to the existing entrypoint script, so the image on dockerhub should now have proper suppoert. Pull the latest main branch and rebase when updating this PR.

@psaxton
Copy link
Contributor Author

psaxton commented Feb 21, 2023

@instantlinux PR has been rebased and updated.
@bhechinger Thanks for the help!

@psaxton
Copy link
Contributor Author

psaxton commented Feb 21, 2023

If there is a lighter weight templater that supports the conditional block I can switch to using that -- I just used j2 because it seems like that was what was originally intended.

@psaxton
Copy link
Contributor Author

psaxton commented Feb 23, 2023

Signed commits as per policy.

Copy link
Owner

@instantlinux instantlinux left a comment

Choose a reason for hiding this comment

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

Compressed image size was 52.02Mb on docker hub before merge; let's see where this ends up after merge. Thanks for the contribution!

@instantlinux instantlinux merged commit 140c9c0 into instantlinux:main Feb 24, 2023
@instantlinux instantlinux changed the title samba-dc actually supports DNS_FORWARDER samba-dc now uses j2 cli command to support DNS_FORWARDER Feb 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants