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

Add tusd official image. #6240

Closed
wants to merge 1 commit into from
Closed

Add tusd official image. #6240

wants to merge 1 commit into from

Conversation

thirsch
Copy link

@thirsch thirsch commented Jul 7, 2019

We'd like to ask you to add our docker image for the TUS reference implementation to the official images repo of docker hub.

If you need more information about the project, image or the group, please feel free to ask.


Checklist for Review

NOTE: This checklist is intended for the use of the Official Images maintainers both to track the status of your PR and to help inform you and others of where we're at. As such, please leave the "checking" of items to the repository maintainers. If there is a point below for which you would like to provide additional information or note completion, please do so by commenting on the PR. Thanks! (and thanks for staying patient with us ❤️)

  • associated with or contacted upstream?
  • does it fit into one of the common categories? ("service", "language stack", "base distribution")
  • is it reasonably popular, or does it solve a particular use case well?
  • does a documentation PR exist? (should be reviewed and merged at roughly the same time so that we don't have an empty image page on the Hub for very long)
  • dockerization review for best practices and cache gotchas/improvements (ala the official review guidelines)?
  • 2+ dockerization review?
  • existing official images have been considered as a base? (ie, if foobar needs Node.js, has FROM node:... instead of grabbing node via other means been considered?)
  • if FROM scratch, tarballs only exist in a single commit within the associated history?
  • passes current tests? any simple new tests that might be appropriate to add? (https://github.com/docker-library/official-images/tree/master/test)

@tianon
Copy link
Member

tianon commented Jul 10, 2019

Unfortunately, as written this is going to be very difficult for us to review since the image context includes the full source code for tusd itself (and so every update PR is going to include the full diff of changes to the upstream source, which isn't relevant for us).

What I'd recommend doing is creating a separate "release" Dockerfile which consumes the official released binaries directly, and proposing that instead (often, image maintainers will put this file into a separate repository so it isn't intertwined with their normal "main" repository merge/review/release processes and can be updated independently).

@tianon
Copy link
Member

tianon commented Jul 11, 2019

Now that I'm back at a computer looking at this, here's an example to help illustrate what I mean:

# TODO switch to Alpine? (requires different binaries to be published)
FROM debian:buster-slim

RUN set -eux; \
	apt-get update; \
	apt-get install -y --no-install-recommends \
		ca-certificates \
		jq \
# TODO move this package down into the download block so we can remove wget after downloading the tusd binary
		wget \
	; \
	rm -rf /var/lib/apt/lists/*

RUN set -eux; \
	groupadd --gid 1000 tusd; \
	useradd --uid 1000 --gid 1000 --shell /bin/bash tusd; \
	mkdir -p /srv/tusd-hooks; \
	mkdir -p /srv/tusd-data; \
	chown tusd:tusd /srv/tusd-data

# https://github.com/tus/tusd/releases
ENV TUSD_VERSION 0.13.0
# (using 0.13.0 since it's the latest release with published artifacts -- looks like 0.13.1 was pretty minor and/or packaging changes anyhow so perhaps why it didn't get new artifacts?)

RUN set -eux; \
	wget -O tusd.tgz "https://github.com/tus/tusd/releases/download/${TUSD_VERSION}/tusd_linux_amd64.tar.gz"; \
# TODO verify some kind of hash; sha256, sha512, etc?  ideally published with the binaries and embedded directly in the Dockerfile itself
# see https://github.com/docker-library/golang/blob/103d42338bd9c3f661ade41f39dbc88fe9dc83a3/1.12/stretch/Dockerfile#L16-L33 for example
	tar -xvf tusd.tgz -C /usr/local/bin/ --strip-components=1 tusd_linux_amd64/tusd; \
	rm tusd.tgz; \
	tusd --version

WORKDIR /srv/tusd-data
EXPOSE 1080
USER tusd

# TODO see https://github.com/docker-library/official-images#consistency and decide whether an ENTRYPOINT would be useful/warranted
CMD ["tusd", "--hooks-dir", "/srv/tusd-hooks"]

As you can see, there are a couple TODO items in there:

  1. my first pass tried to use Alpine, but doesn't work because the published binaries are dynamically linked, not static; I'm not sure how those are being built today, but I'd recommend considering in the future using more flags on go build to help make them fully static so they could be used in any distro as-is (I like -v -ldflags '-d -s -w', but there are less broad sets that can achieve a similar result, and if you use net anywhere you'll also need something like -tags=netgo -installsuffix=netgo to ensure the net library is pure-Go

  2. moving to Alpine would resolve wget automatically (since busybox's wget would be sufficient and removing it would thus be a moot point)

  3. I'd recommend publishing some kind of hash with each binary release, and embedding those directly in the Dockerfile itself for verification (see https://github.com/docker-library/golang/blob/103d42338bd9c3f661ade41f39dbc88fe9dc83a3/1.12/stretch/Dockerfile#L16-L33 for an example of what that might look like, especially if you wanted to support multiple architectures)

  4. ENTRYPOINT ["tusd"] won't be acceptable as-is, but something that emulates it is possible; see https://github.com/docker-library/official-images#consistency for more details

  5. regarding arm, you'll want to make sure your build process is explicitly setting GOARM and note which ARM variant it is (see https://github.com/golang/go/wiki/GoArm for more on that, especially "it is recommended that you always set an appropriate GOARM value along with GOARCH"); see https://golang.org/dl/ with armv6l for example

@thirsch
Copy link
Author

thirsch commented Jul 11, 2019

Thank you very much @tianon for your detailed answer. It is very helpful to me, as this is my first official image attempt!

I'll discuss and work on the points within the tusd team.

@github-actions
Copy link

github-actions bot commented May 5, 2020

Diff for a8143b7:
TODO diff too large for GitHub comment!
See: http://github.com/docker-library/official-images/actions/runs/96647399

@tianon
Copy link
Member

tianon commented May 6, 2020

Friendly ping?

@thirsch
Copy link
Author

thirsch commented May 9, 2020

Thanks for the ping. Would it be ok to start with a first version pretty much based on your template using buster-slim and only amd64 and come up with further optimisations on a later point?

@tianon
Copy link
Member

tianon commented Jun 24, 2020

Sorry for the delay 🙇

Yes, just having a single variant based on what I've proposed would be a fine place to begin. 👍

@yosifkit
Copy link
Member

Closing given absence of activity. We apologize for the delays with our responses. 🙇‍♂️ Not to excuse our slow or missing replies, we often have to prioritize maintaining the current images over adding new images. Thanks for your contribution.

If you'd still like to propose a new image, let us know here and we'll try to help it move along. If you've since moved on to other endeavors, we wish you the best. ❤️

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

Successfully merging this pull request may close these issues.

4 participants