Skip to content

add Dockerfile for Docker support#31

Open
iambryancs wants to merge 3 commits into
jetmore:developfrom
iambryancs:develop
Open

add Dockerfile for Docker support#31
iambryancs wants to merge 3 commits into
jetmore:developfrom
iambryancs:develop

Conversation

@iambryancs

@iambryancs iambryancs commented Jan 20, 2021

Copy link
Copy Markdown

Hi John, this PR adds support for Docker. Very useful in my case where I do tests on dev k8s cluster.
Also useful on any system with Docker that don't want to deal with installing CPAN/Perl and other extensions like Net-SSLeay.

Usage:

docker run --rm -ti jetmore/swaks [OPTIONS]

Example:

# test
docker run --rm -ti jetmore/swaks \
  -f foo@bar.com \
  -t foo@baz.com \
  -s localhost

# help
docker run --rm jetmore/swaks --help

@iambryancs iambryancs changed the title add Dockerfile add Dockerfile for Docker support Jan 20, 2021
@mathieu-aubin

Copy link
Copy Markdown

considering this is a single file, standalone... do you think it is needed to have such docker setup?

@jetmore

jetmore commented Sep 14, 2021

Copy link
Copy Markdown
Owner

considering this is a single file, standalone... do you think it is needed to have such docker setup?

The script itself is only one file, but there are optional perl modules (as referenced by the ticket creator) that could be pulled in. Also, I imagine in some deploy environments that are optimized for Docker it would be nice to have even for a single file. I haven't thought yet about how to formally include this in the project but I support the general idea.

@mathieu-aubin

Copy link
Copy Markdown

fair enough

@jetmore jetmore added the enhancement New functionality slated to be implemented someday label Nov 5, 2023
@jetmore jetmore added this to the backlog milestone Nov 13, 2023
@Neustradamus

Copy link
Copy Markdown

@jetmore: Any progress on this PR?

@iambryancs

Copy link
Copy Markdown
Author

@jetmore: Any progress on this PR?

@Neustradamus , thank you for reminding. I have completely forgotten this PR.

Anyway, I added a new commit to accommodate the latest changes if in case @jetmore decides to approve this.

@polarathene polarathene left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

If review does ever progress, here's my feedback.

Comment thread Dockerfile
Comment on lines +3 to +4
LABEL maintainer="John Jetmore <@jetmore>"
LABEL maintainer="Bryan CS <@iambryancs>"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Not an appropriate LABEL, stick to the standard annotations.

Suggested change
LABEL maintainer="John Jetmore <@jetmore>"
LABEL maintainer="Bryan CS <@iambryancs>"
LABEL org.opencontainers.image.authors="John Jetmore <@jetmore>, Bryan CS <@iambryancs>"

Comment thread Dockerfile
LABEL maintainer="John Jetmore <@jetmore>"
LABEL maintainer="Bryan CS <@iambryancs>"

ENV RELEASE=20240103.0

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This does not need to persist into the image at runtime as an ENV, use ARG which CI can target to dynamically adjust if the default value assigned (optional) is not desired.

Suggested change
ENV RELEASE=20240103.0
ARG RELEASE=20240103.0

Comment thread Dockerfile

ENV RELEASE=20240103.0

RUN apk add --update perl curl perl-net-ssleay

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

There is no package index at this point to update, it'll be updated/fetched regardless so drop --update:

Suggested change
RUN apk add --update perl curl perl-net-ssleay
RUN apk add curl perl perl-net-ssleay

Comment thread Dockerfile
Comment on lines +10 to +12
RUN curl -O https://www.jetmore.org/john/code/swaks/files/swaks-${RELEASE}/swaks

RUN chmod +x ./swaks

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Using a separate RUN for chmod duplicates the file weight in the image, even though only a tiny portion of it was actually modified, the full file doubles in weight.


If this is just to add a single file, you can avoid the curl dep and just use:

Suggested change
RUN curl -O https://www.jetmore.org/john/code/swaks/files/swaks-${RELEASE}/swaks
RUN chmod +x ./swaks
ADD --chmod=+x https://www.jetmore.org/john/code/swaks/files/swaks-${RELEASE}/swaks /usr/local/bin/swaks
  • Placing the file in /usr/local/bin/ should be in the containers PATH ENV, in which case this will resolve from anywhere without needing a relative path.
  • --chmod=+x is specific to BuildKit support IIRC (I think Podman lacks it for example), you could just set the octal value here for better compatibility (eg: --chmod=0755).

Alternatively just merge the two RUN into a multi-line:

Suggested change
RUN curl -O https://www.jetmore.org/john/code/swaks/files/swaks-${RELEASE}/swaks
RUN chmod +x ./swaks
RUN curl -fsSLO https://www.jetmore.org/john/code/swaks/files/swaks-${RELEASE}/swaks && \
chmod +x ./swaks

Comment thread README.md
Comment on lines +22 to +37
Usage:
```
docker run --rm -ti bryancs/swaks [OPTIONS]
```

Example:
```
# test
docker run --rm -ti bryancs/swaks \
-f foo@bar.com \
-t foo@baz.com \
-s localhost

# help
docker run --rm bryancs/swaks --help
```

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

If you are going to suggest users to run your image at DockerHub like that, there should at least be disclosure that it's an unofficial third-party image to pull, not one endorsed by the project 😓

That said looking at bryancs/swaks on DockerHub the last image pushed there was over 5 years ago (Jan 2021), which I don't think is what users would expect.

Without the project adopting CI to build and publish an official image to GHCR (easiest), it's probably better to advise users to build the image locally (and optionally inform of your DockerHub image, if it were to be updated - along with disclosure that it's an unofficial image).

At this stage I wouldn't bother attempting to contribute the Github Actions workflow, since there's enough friction to get the current PR merged, best not to burden/overwhelm the maintainer any further.

Also note that the example would not work either, as localhost within a container is going to point to it's own loopback where nothing is running.

Suggested change
Usage:
```
docker run --rm -ti bryancs/swaks [OPTIONS]
```
Example:
```
# test
docker run --rm -ti bryancs/swaks \
-f foo@bar.com \
-t foo@baz.com \
-s localhost
# help
docker run --rm bryancs/swaks --help
```
Build the image:
```bash
git clone https://github.com/jetmore/swaks && cd swaks
docker build --tag jetmore/swaks .
```
Run the container with a typical `swaks` command:
```bash
docker run --rm -it jetmore/swaks \
--from john.doe@example.com \
--to jane.doe@example.test \
--server mail.example.com
```

@polarathene

Copy link
Copy Markdown

My personal opinion is an official image while nice isn't that important to maintain for the project.

The Dockerfile can still be beneficial, but given the release cadence over the years, there would be very little delay to just using the standard Alpine image and apk add swaks (might not have been an option back in 2021 when this PR started).

Anyone who'd build an image could then do the same thing, so the only real advantage in that scenario is the small window of time before Alpine packages the newer release of swaks, or if an official image was published to a container registry like GHCR it'd have that slight convenience of not having to build an image locally (or run a disposable alpine container that installs the distro package of swaks, install overhead is rather low).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New functionality slated to be implemented someday

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants